On Wed, 3 Jun 2020 at 10:16, Cédric Le Goater <c...@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.s...@gmail.com > > <mailto:erik.lucas.s...@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