Re: [Qemu-arm] [PATCH v3 07/30] imx_fec: Add support for multiple Tx DMA rings

[Andrey Smirnov]

On Tue, Nov 21, 2017 at 9:44 AM, Peter Maydell <address@hidden> wrote:
> On 6 November 2017 at 15:47, Andrey Smirnov <address@hidden> wrote:
>> More recent version of the IP block support more than one Tx DMA ring,
>> so add the code implementing that feature.
>>
>> Cc: Peter Maydell <address@hidden>
>> Cc: Jason Wang <address@hidden>
>> Cc: Philippe Mathieu-Daudé <address@hidden>
>> Cc: address@hidden
>> Cc: address@hidden
>> Cc: address@hidden
>> Signed-off-by: Andrey Smirnov <address@hidden>
>
>>  static const VMStateDescription vmstate_imx_eth = {
>>      .name = TYPE_IMX_FEC,
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> +    .version_id = 3,
>> +    .minimum_version_id = 3,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX),
>>          VMSTATE_UINT32(rx_descriptor, IMXFECState),
>> -        VMSTATE_UINT32(tx_descriptor, IMXFECState),
>> -
>> +        VMSTATE_UINT32_ARRAY(tx_descriptor, IMXFECState, ENET_TX_RING_NUM),
>> +        VMSTATE_UINT32(tx_ring_num, IMXFECState),
>>          VMSTATE_UINT32(phy_status, IMXFECState),
>>          VMSTATE_UINT32(phy_control, IMXFECState),
>>          VMSTATE_UINT32(phy_advertise, IMXFECState),
>
> tx_ring_num is constant for any particular instantiation of the device,
> so you don't need to put it in the vmstate.
>
> It's pretty trivial to make this vmstate compatible with the old
> ones for the existing single-tx-descriptor devices, so we might as well:
>
> /* Versions of this device with more than one TX descriptor
>  * save the 2nd and 3rd descriptors in a subsection, to maintain
>  * migration compatibility with previous versions of the device
>  * that only supported a single descriptor.
>  */
> static bool txdescs_needed(void *opaque) {
>     IMXFECState *s = opaque;
>
>     return s->tx_ring_num > 1;
> }
>
> static const VMStateDescription vmstate_imx_eth_txdescs = {
>     .name = "imx.fec/txdescs",
>     .version_id = 1,
>     .minimum_version_id = 1,
>     .needed = txdescs_needed,
>     .fields = (VMStateField[]) {
>          VMSTATE_UINT32(tx_descriptor[1], IMXFECState),
>          VMSTATE_UINT32(tx_descriptor[2], IMXFECState),
>          VMSTATE_END_OF_LIST()
>     }
> };
>
> and then add this to the vmx_state_eth at the end:
>     .subsections = (const VMStateDescription*[]) {
>          &vmstate_imx_eth_txdescs,
>          NULL
>     }
>
> Then you don't need to bump version_id/minimum_version_id on the
> vmstate_imx_eth struct.
>

Cool, sounds good. Will add that to the patch in v4.

>> @@ -791,6 +821,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, 
>> uint64_t value,
>>                             unsigned size)
>>  {
>>      IMXFECState *s = IMX_FEC(opaque);
>> +    const bool single_tx_ring = s->tx_ring_num != 3;
>
> This looks odd -- I would have expected "single_tx_ring =
> s->tx_ring_num == 1" ...

AFAIK the HW that's out there will have either 3 or 1 Tx ring, so
that's why I wrote it this way. I'll change the logic to the way your
suggest to avoid surprising the reader.

Thanks,
Andrey Smirnov