[PATCH v2] Implement configurable descriptor size in ftgmac100

Erik Smit posted 1 patch 3 years, 11 months ago
Failed in applying to current master (apply log)
hw/net/ftgmac100.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
[PATCH v2] Implement configurable descriptor size in ftgmac100
Posted by Erik Smit 3 years, 11 months ago
The hardware supports configurable descriptor sizes, configured in the DBLAC
register.

Most drivers use the default 4 word descriptor, which is currently hardcoded,
but Aspeed SDK configures 8 words to store extra data.

---
The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
word entries:
https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.h#L387-L391

And sets DBLAC to 0x44f97:
https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.c#L449

There's not a lot of public documentation on this hardware, but the
current linux driver shows the meaning of these registers:

https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/faraday/ftgmac100.c#L280-L281

        iowrite32(FTGMAC100_DBLAC_RXDES_SIZE(2) |   /* 2*8 bytes RX descs */
                  FTGMAC100_DBLAC_TXDES_SIZE(2) |   /* 2*8 bytes TX descs */

Without this patch, networking in SMT_X11_158 fails after the first packet.

changes since previous version:

- moved "* 8" into {R,T}XDES_SIZE macro
- removed the RXFIFO and RX_THR_EN defines as they're AST2400-only and not used
- test setting of DBLAC register for validness

Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com>
---
 hw/net/ftgmac100.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25ebee7ec2..83058497c4 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -79,6 +79,16 @@
 #define FTGMAC100_APTC_TXPOLL_CNT(x)        (((x) >> 8) & 0xf)
 #define FTGMAC100_APTC_TXPOLL_TIME_SEL      (1 << 12)

+/*
+ * DMA burst length and arbitration control register
+ */
+#define FTGMAC100_DBLAC_RXBURST_SIZE(x)     (((x) >> 8) & 0x3)
+#define FTGMAC100_DBLAC_TXBURST_SIZE(x)     (((x) >> 10) & 0x3)
+#define FTGMAC100_DBLAC_RXDES_SIZE(x)       ((((x) >> 12) & 0xf) * 8)
+#define FTGMAC100_DBLAC_TXDES_SIZE(x)       ((((x) >> 16) & 0xf) * 8)
+#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
+#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
+
 /*
  * PHY control register
  */
@@ -553,7 +563,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s,
uint32_t tx_ring,
         if (bd.des0 & s->txdes0_edotr) {
             addr = tx_ring;
         } else {
-            addr += sizeof(FTGMAC100Desc);
+            addr += (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac));
         }
     }

@@ -800,6 +810,18 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
         s->phydata = value & 0xffff;
         break;
     case FTGMAC100_DBLAC: /* DMA Burst Length and Arbitration Control */
+        if (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: transmit descriptor too small : %d bytes\n",
+                          __func__, FTGMAC100_DBLAC_TXDES_SIZE(s->dblac));
+            break;
+        }
+        if (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: receive descriptor too small : %d bytes\n",
+                          __func__, FTGMAC100_DBLAC_RXDES_SIZE(s->dblac));
+            break;
+        }
         s->dblac = value;
         break;
     case FTGMAC100_REVR:  /* Feature Register */
@@ -982,7 +1004,7 @@ static ssize_t ftgmac100_receive(NetClientState
*nc, const uint8_t *buf,
         if (bd.des0 & s->rxdes0_edorr) {
             addr = s->rx_ring;
         } else {
-            addr += sizeof(FTGMAC100Desc);
+            addr += (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac));
         }
     }
     s->rx_descriptor = addr;
--
2.25.1

Re: [PATCH v2] Implement configurable descriptor size in ftgmac100
Posted by Cédric Le Goater 3 years, 11 months ago
On 6/6/20 11:03 AM, Erik Smit wrote:
> The hardware supports configurable descriptor sizes, configured in the DBLAC
> register.
> 
> Most drivers use the default 4 word descriptor, which is currently hardcoded,
> but Aspeed SDK configures 8 words to store extra data.
> 
> ---
> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
> word entries:
> https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.h#L387-L391
> 
> And sets DBLAC to 0x44f97:
> https://github.com/ya-mouse/openwrt-linux-aspeed/blob/master/drivers/net/ftgmac100_26.c#L449
> 
> There's not a lot of public documentation on this hardware, but the
> current linux driver shows the meaning of these registers:
> 
> https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/faraday/ftgmac100.c#L280-L281
> 
>         iowrite32(FTGMAC100_DBLAC_RXDES_SIZE(2) |   /* 2*8 bytes RX descs */
>                   FTGMAC100_DBLAC_TXDES_SIZE(2) |   /* 2*8 bytes TX descs */
> 
> Without this patch, networking in SMT_X11_158 fails after the first packet.
> 
> changes since previous version:
> 
> - moved "* 8" into {R,T}XDES_SIZE macro
> - removed the RXFIFO and RX_THR_EN defines as they're AST2400-only and not used
> - test setting of DBLAC register for validness
> 
> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com>

We don't need the parenthesis around FTGMAC100_DBLAC_RXDES_SIZE(). Anyhow,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  hw/net/ftgmac100.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 25ebee7ec2..83058497c4 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -79,6 +79,16 @@
>  #define FTGMAC100_APTC_TXPOLL_CNT(x)        (((x) >> 8) & 0xf)
>  #define FTGMAC100_APTC_TXPOLL_TIME_SEL      (1 << 12)
> 
> +/*
> + * DMA burst length and arbitration control register
> + */
> +#define FTGMAC100_DBLAC_RXBURST_SIZE(x)     (((x) >> 8) & 0x3)
> +#define FTGMAC100_DBLAC_TXBURST_SIZE(x)     (((x) >> 10) & 0x3)
> +#define FTGMAC100_DBLAC_RXDES_SIZE(x)       ((((x) >> 12) & 0xf) * 8)
> +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       ((((x) >> 16) & 0xf) * 8)
> +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
> +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
> +
>  /*
>   * PHY control register
>   */
> @@ -553,7 +563,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s,
> uint32_t tx_ring,
>          if (bd.des0 & s->txdes0_edotr) {
>              addr = tx_ring;
>          } else {
> -            addr += sizeof(FTGMAC100Desc);
> +            addr += (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac));
>          }
>      }
> 
> @@ -800,6 +810,18 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>          s->phydata = value & 0xffff;
>          break;
>      case FTGMAC100_DBLAC: /* DMA Burst Length and Arbitration Control */
> +        if (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: transmit descriptor too small : %d bytes\n",
> +                          __func__, FTGMAC100_DBLAC_TXDES_SIZE(s->dblac));
> +            break;
> +        }
> +        if (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: receive descriptor too small : %d bytes\n",
> +                          __func__, FTGMAC100_DBLAC_RXDES_SIZE(s->dblac));
> +            break;
> +        }
>          s->dblac = value;
>          break;
>      case FTGMAC100_REVR:  /* Feature Register */
> @@ -982,7 +1004,7 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
>          if (bd.des0 & s->rxdes0_edorr) {
>              addr = s->rx_ring;
>          } else {
> -            addr += sizeof(FTGMAC100Desc);
> +            addr += (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac));
>          }
>      }
>      s->rx_descriptor = addr;
> --
> 2.25.1
> 


Re: [PATCH v2] Implement configurable descriptor size in ftgmac100
Posted by Peter Maydell 3 years, 10 months ago
On Sat, 6 Jun 2020 at 10:03, Erik Smit <erik.lucas.smit@gmail.com> wrote:
>
> The hardware supports configurable descriptor sizes, configured in the DBLAC
> register.
>
> Most drivers use the default 4 word descriptor, which is currently hardcoded,
> but Aspeed SDK configures 8 words to store extra data.

Hi; I've applied this to target-arm.next, with the parenthesis
change that Cédric suggested; thanks for your contribution.

A couple of minor patch format notes, which are only worth worrying
about if you plan to submit more QEMU patches in future:

> ---

The Signed-off-by: line should go above this '---' divider, because
it wants to go into the commit message in git; anything below '---'
is discarded when the patch is applied.

>  /*
>   * PHY control register
>   */
> @@ -553,7 +563,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s,
> uint32_t tx_ring,

Your email client is wrapping long lines, which corrupts patches.
(You can see that patchew was unable to apply and CI-test it:
https://patchew.org/QEMU/CA+MHfovYq1UAQQ341MnyGas7ScskWyjR=QE0D-RS7+zxF2mtsw@mail.gmail.com/
)

I fixed this patch up by hand, but if you're planning on sending
more patches in future it would be worth sorting out how to send them
in a way that doesn't wrap them. (Most long-term devs use some form
of the 'git send-email' client.)

thanks
-- PMM