Re: [Qemu-arm] [Qemu-devel] [PATCH v2 3/6] cadence_gem: Add support for screening

[Alistair Francis]

On Tue, Jul 26, 2016 at 9:55 AM, Peter Maydell <address@hidden> wrote:
> On 26 July 2016 at 17:42, Alistair Francis <address@hidden> wrote:
>> On Tue, Jul 26, 2016 at 5:01 AM, Peter Maydell <address@hidden> wrote:
>>> On 26 July 2016 at 01:12, Alistair Francis <address@hidden> wrote:
>>>> The Cadence GEM hardware allows incoming data to be 'screened' based on 
>>>> some
>>>> register values. Add support for these screens.
>>>>
>>>> Signed-off-by: Alistair Francis <address@hidden>
>
>>>> +static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr)
>>>
>>> Nothing seems to call this -- this probably results in a complaint
>>> about an unused function if you build at this point in the series
>>> (possibly only with optimisation on).
>>
>> There is a warning about this. I wasn't sure what the position on
>> warnings in between patch a series was. I can squash this into the
>> next patch, I was just worried that the patch was getting a little
>> big.
>>
>> What do you think?
>
> Warnings are compile failures by default, so they break bisection.
> That makes them worth avoiding.
>
> Here's a rearrangement that I think should work, though it's
> a little tedious:
>
> (1) change patch 2/6 so that instead of using hardcoded [0] for
> the array dereferences it uses [q], with an "int q = 0;" at the
> top of the relevant functions (gem_transmit and gem_receive).
> (This will also reduce churn in patch 4 since we just go from
> foo to foo[q] rather than foo to foo[0] to foo[q].)
>
> (2) Then this patch can switch the 'q = 0' to the
> + /* Find which queue we are targetting */
> + q = get_queue_from_screen(s, rxbuf_ptr);
>
> that's currently in patch 4. (It'll always return 0 at this point,
> since the registers can't be written by the guest yet.)

I was hoping to avoid that, but it actually wasn't too bad. Sending
the next version now.

Thanks,

Alistair

>
>>>> +            offset = extract32(cr1, GEM_T2CW1_OFFSET_VALUE_SHIFT,
>>>> +                                    GEM_T2CW1_OFFSET_VALUE_WIDTH);
>>>> +
>>>> +            switch (extract32(cr1, GEM_T2CW1_COMPARE_OFFSET_SHIFT,
>>>> +                                   GEM_T2CW1_COMPARE_OFFSET_WIDTH)) {
>>>
>>> You pulled this out into 'offset' so you can just switch (offset).
>>
>> They are actually different.
>
> Oops, so they are...
>
> thanks
> -- PMM
>