[PATCH] ftgmac100: Implement variable descriptor size

[PATCH] ftgmac100: Implement variable descriptor size

[Erik Smit]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

The hardware supports variable descriptor sizes, configured with the DBLAC
register.

Most drivers use the default 2*8, which is currently hardcoded in qemu, but
the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.

--
The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
4-bytes 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 does not pass data.

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

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25ebee7ec2..1640b24b23 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -79,6 +79,19 @@
 #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
+#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
+#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)
+#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)
+#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)
+#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
+#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
+
 /*
  * PHY control register
  */
@@ -553,7 +566,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)) * 8;
         }
     }

@@ -982,7 +995,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)) * 8;
         }
     }
     s->rx_descriptor = addr;
--
2.25.1

[-- Attachment #2: Type: text/html, Size: 3645 bytes --]

Re: [PATCH] ftgmac100: Implement variable descriptor size

[Philippe Mathieu-Daudé]

On 6/2/20 6:47 PM, Erik Smit wrote:
> The hardware supports variable descriptor sizes, configured with the DBLAC
> register.
> 
> Most drivers use the default 2*8, which is currently hardcoded in qemu, but
> the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.
> 
> --
> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
> 4-bytes 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 does not pass data.

Does it really 'pass' *all* the data?

This patch seems incomplete...

IMO you should 1/ declare FTGMAC100Desc as:

typedef struct {
    uint32_t        des0;
    uint32_t        des1;
} FTGMAC100Desc;

2/ Replace the code using static '2' by dynamic use of
FTGMAC100_DBLAC_xXDES_SIZE(dblac):

  static int ftgmac100_read_bd(FTGMAC100Desc **bd, dma_addr_t addr)
  {
      unsigned bd_idx;

      if (dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd))) {
          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read descriptor
@ 0x%"
                        HWADDR_PRIx "\n", __func__, addr);
          return -1;
      }
      for (bd_idx = 0; bd_idx< FTGMAC100_DBLAC_RXDES_SIZE(s->dblac);
bd_idx++) {
          bd[bd_idx]->des0 = le32_to_cpu(bd[bd_idx]->des0);
          bd[bd_idx]->des1 = le32_to_cpu(bd[bd_idx]->des1);
      }

      return 0;
  }

Etc...

Maybe worth introduce the bd_to_cpu()/cpu_to_bd() helpers too
(respectively calling le32_to_cpu & cpu_to_le32).

> 
> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com
> <mailto:erik.lucas.smit@gmail.com>>
> ---
>  hw/net/ftgmac100.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 25ebee7ec2..1640b24b23 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -79,6 +79,19 @@
>  #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
> +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
> +#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)
> +#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)
> +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)
> +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
> +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
> +
>  /*
>   * PHY control register
>   */
> @@ -553,7 +566,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)) * 8;

Extra parenthesis not needed.

After doing 1/ you can now replace '8' by sizeof(FTGMAC100Desc).

>          }
>      }
> 
> @@ -982,7 +995,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)) * 8;
>          }
>      }
>      s->rx_descriptor = addr;
> --
> 2.25.1

Re: [PATCH] ftgmac100: Implement variable descriptor size

[Cédric Le Goater]

On 6/3/20 9:08 AM, Philippe Mathieu-Daudé wrote:
> On 6/2/20 6:47 PM, Erik Smit wrote:
>> The hardware supports variable descriptor sizes, configured with the DBLAC
>> register.
>>
>> Most drivers use the default 2*8, which is currently hardcoded in qemu, but
>> the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.
>>
>> --
>> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
>> 4-bytes 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 does not pass data.
> 
> Does it really 'pass' *all* the data?
> 
> This patch seems incomplete...
> 
> IMO you should 1/ declare FTGMAC100Desc as:
> 
> typedef struct {
>     uint32_t        des0;
>     uint32_t        des1;
> } FTGMAC100Desc;

The TX and RX descriptors have 4 words in the architecture which are
used by HW but software can use bigger size for its own purpose. This 
is what is doing the Aspeed SDK.

> 2/ Replace the code using static '2' by dynamic use of
> FTGMAC100_DBLAC_xXDES_SIZE(dblac):
> 
>   static int ftgmac100_read_bd(FTGMAC100Desc **bd, dma_addr_t addr)
>   {
>       unsigned bd_idx;
> 
>       if (dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd))) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read descriptor
> @ 0x%"
>                         HWADDR_PRIx "\n", __func__, addr);
>           return -1;
>       }
>       for (bd_idx = 0; bd_idx< FTGMAC100_DBLAC_RXDES_SIZE(s->dblac);
> bd_idx++) {
>           bd[bd_idx]->des0 = le32_to_cpu(bd[bd_idx]->des0);
>           bd[bd_idx]->des1 = le32_to_cpu(bd[bd_idx]->des1);
>       }
> 
>       return 0;
>   }
> 
> Etc...
> 

The current ftgmac100_read_bd() and ftgmac100_write_bd() routines model 
the HW perspective and they are fine as they are I think.

C.

> Maybe worth introduce the bd_to_cpu()/cpu_to_bd() helpers too
> (respectively calling le32_to_cpu & cpu_to_le32).
>>
>> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com
>> <mailto:erik.lucas.smit@gmail.com>>
>> ---
>>  hw/net/ftgmac100.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 25ebee7ec2..1640b24b23 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -79,6 +79,19 @@
>>  #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
>> +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
>> +#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)
>> +#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)
>> +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)
>> +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
>> +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
>> +
>>  /*
>>   * PHY control register
>>   */
>> @@ -553,7 +566,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)) * 8;
> 
> Extra parenthesis not needed.
> 
> After doing 1/ you can now replace '8' by sizeof(FTGMAC100Desc).
> 
>>          }
>>      }
>>
>> @@ -982,7 +995,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)) * 8;
>>          }
>>      }
>>      s->rx_descriptor = addr;
>> --
>> 2.25.1
> 

Re: [PATCH] ftgmac100: Implement variable descriptor size

[Cédric Le Goater]

On 6/2/20 6:47 PM, Erik Smit wrote:
> The hardware supports variable descriptor sizes, configured with the DBLAC
> register.

yes.

The DBLAC default value is 0x00022F00 on AST2400 and 0x00022500 on AST2500 
and AST2600. The current reset handler needs a little fix btw.

This sets the TX and RX descriptor default size to 4 words (2 * 8bytes). 

> Most drivers use the default 2*8, which is currently hardcoded in qemu, but
> the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.

The first 4 words are architected but the specs allows the descriptors 
to be bigger which is what the Aspeed SDK is doing:

	outl( 0x44f97, dev->base_addr + DBLAC_REG );

It's using 8 words ( 4 * 8bytes) to store some address in the fifth. 
This is a waste btw.


Thanks for spotting this. I think the patch is correct but we need to 
clarify a few things. 

> --
> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
> 4-bytes 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 does not pass data.
> 
> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com <mailto:erik.lucas.smit@gmail.com>>
> ---
>  hw/net/ftgmac100.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 25ebee7ec2..1640b24b23 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -79,6 +79,19 @@
>  #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
> +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
> +#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)

The above definitions are AST2400 only. We should say so or leave them out 
because the model does not use them any how.

> +#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)
> +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)

I would include '* 8' in the {R,T}XDES_SIZE macros

> +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
> +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
> +
>  /*
>   * PHY control register
>   */
> @@ -553,7 +566,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)) * 8;

and remove the '* 8' here.

>          }
>      }
> 
> @@ -982,7 +995,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)) * 8;
>          }
>      }
>      s->rx_descriptor = addr;


and when the DBLAC register is set, we should check the size values to make 
sure they are not under sizeof(FTGMAC100Desc), in which case we should 
report an error.

Thanks,

C.

Re: [PATCH] ftgmac100: Implement variable descriptor size

[Erik Smit]

On Wed, 3 Jun 2020 at 10:16, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 6/2/20 6:47 PM, Erik Smit wrote:
> > The hardware supports variable descriptor sizes, configured with the DBLAC
> > register.
>
> yes.
>
> The DBLAC default value is 0x00022F00 on AST2400 and 0x00022500 on AST2500
> and AST2600. The current reset handler needs a little fix btw.
>
> This sets the TX and RX descriptor default size to 4 words (2 * 8bytes).
>
> > Most drivers use the default 2*8, which is currently hardcoded in qemu, but
> > the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.
>
> The first 4 words are architected but the specs allows the descriptors
> to be bigger which is what the Aspeed SDK is doing:
>
>         outl( 0x44f97, dev->base_addr + DBLAC_REG );
>
> It's using 8 words ( 4 * 8bytes) to store some address in the fifth.
> This is a waste btw.
>
>
> Thanks for spotting this. I think the patch is correct but we need to
> clarify a few things.
>
> > --
> > The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
> > 4-bytes 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 does not pass data.
> >
> > Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com <mailto:erik.lucas.smit@gmail.com>>
> > ---
> >  hw/net/ftgmac100.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > index 25ebee7ec2..1640b24b23 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -79,6 +79,19 @@
> >  #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
> > +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
> > +#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)
>
> The above definitions are AST2400 only. We should say so or leave them out
> because the model does not use them any how.

Like so?

#define FTGMAC100_DBLAC_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)  // AST2400-only
#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)  // AST2400-only
#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)            // AST2400-only

>
> > +#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)
> > +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)
>
> I would include '* 8' in the {R,T}XDES_SIZE macros

Agreed.

> > +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
> > +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
> > +
> >  /*
> >   * PHY control register
> >   */
> > @@ -553,7 +566,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)) * 8;
>
> and remove the '* 8' here.

Agreed.

> >          }
> >      }
> >
> > @@ -982,7 +995,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)) * 8;
> >          }
> >      }
> >      s->rx_descriptor = addr;
>
>
> and when the DBLAC register is set, we should check the size values to make
> sure they are not under sizeof(FTGMAC100Desc), in which case we should
> report an error.

Like so?

    case FTGMAC100_DBLAC: /* DMA Burst Length and Arbitration Control */
        s->dblac = value;
        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));
        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;

Also, I've not got experience submitting patches to Qemu. My next step
would be to respin this patch and resend it to everybody as [PATCH
v2]?

Best regards,

Erik Smit

Re: [PATCH] ftgmac100: Implement variable descriptor size

[Cédric Le Goater]

On 6/4/20 12:54 PM, Erik Smit wrote:
> On Wed, 3 Jun 2020 at 10:16, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 6/2/20 6:47 PM, Erik Smit wrote:
>>> The hardware supports variable descriptor sizes, configured with the DBLAC
>>> register.
>>
>> yes.
>>
>> The DBLAC default value is 0x00022F00 on AST2400 and 0x00022500 on AST2500
>> and AST2600. The current reset handler needs a little fix btw.
>>
>> This sets the TX and RX descriptor default size to 4 words (2 * 8bytes).
>>
>>> Most drivers use the default 2*8, which is currently hardcoded in qemu, but
>>> the implementation of the driver in Supermicro BMC SMT_X11_158 uses 4*8.
>>
>> The first 4 words are architected but the specs allows the descriptors
>> to be bigger which is what the Aspeed SDK is doing:
>>
>>         outl( 0x44f97, dev->base_addr + DBLAC_REG );
>>
>> It's using 8 words ( 4 * 8bytes) to store some address in the fifth.
>> This is a waste btw.
>>
>>
>> Thanks for spotting this. I think the patch is correct but we need to
>> clarify a few things.
>>
>>> --
>>> The implementation of the driver in Supermicro BMC SMT_X11_158 adds 4 extra
>>> 4-bytes 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 does not pass data.
>>>
>>> Signed-off-by: Erik Smit <erik.lucas.smit@gmail.com <mailto:erik.lucas.smit@gmail.com>>
>>> ---
>>>  hw/net/ftgmac100.c | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>>> index 25ebee7ec2..1640b24b23 100644
>>> --- a/hw/net/ftgmac100.c
>>> +++ b/hw/net/ftgmac100.c
>>> @@ -79,6 +79,19 @@
>>>  #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_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)
>>> +#define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)
>>> +#define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)
>>
>> The above definitions are AST2400 only. We should say so or leave them out
>> because the model does not use them any how.
> 
> Like so?
> 
> #define FTGMAC100_DBLAC_RXFIFO_LTHR(x)      (((x) >> 0) & 0x7)  // AST2400-only
> #define FTGMAC100_DBLAC_RXFIFO_HTHR(x)      (((x) >> 3) & 0x7)  // AST2400-only
> #define FTGMAC100_DBLAC_RX_THR_EN           (1 << 6)            // AST2400-only

yes, but without the C++ comments. Keep the number of chars below 
80 also.

>>
>>> +#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)
>>> +#define FTGMAC100_DBLAC_TXDES_SIZE(x)       (((x) >> 16) & 0xf)
>>
>> I would include '* 8' in the {R,T}XDES_SIZE macros
> 
> Agreed.
> 
>>> +#define FTGMAC100_DBLAC_IFG_CNT(x)          (((x) >> 20) & 0x7)
>>> +#define FTGMAC100_DBLAC_IFG_INC             (1 << 23)
>>> +
>>>  /*
>>>   * PHY control register
>>>   */
>>> @@ -553,7 +566,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)) * 8;
>>
>> and remove the '* 8' here.
> 
> Agreed.
> 
>>>          }
>>>      }
>>>
>>> @@ -982,7 +995,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)) * 8;
>>>          }
>>>      }
>>>      s->rx_descriptor = addr;
>>
>>
>> and when the DBLAC register is set, we should check the size values to make
>> sure they are not under sizeof(FTGMAC100Desc), in which case we should
>> report an error.
> 
> Like so?

yes.

To be precise, according to the specs :

    Writing 0 to this field is illegal.

which means that the HW can survive with only 2 words in the TX and RX 
descriptors, word0 and word1. word2 is reserved and word3 is architected 
to point to the TX and RX buffers. Without word3, the HW is dropping all
send and received data, which is a bit useless and the receive part in 
the model doesn't support that case. 

I think testing against sizeof(FTGMAC100Desc) is safer for the time being,

> 
>     case FTGMAC100_DBLAC: /* DMA Burst Length and Arbitration Control */
>         s->dblac = value;

I would move the assignment after the tests. 

>         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));
>         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;
you need opening and closing braces and to break out from the switch 
statement in case of error.

> Also, I've not got experience submitting patches to Qemu. My next step
> would be to respin this patch and resend it to everybody as [PATCH
> v2]?

yes. Take a look at : 

	https://wiki.qemu.org/Contribute/SubmitAPatch

The minimum is to run ./scripts/checkpatch.pl before sending any patch. 
The script will tell you what's wrong.

Thanks,

C.