Re: [PATCH] ftgmac100: Implement variable descriptor size

["Cédric Le Goater" <clg@kaod.org>]

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.