Re: [PATCH] ftgmac100: Implement variable descriptor size

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

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.