qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v2 3/6] cadence_gem: Add support for screening
Date: Tue, 26 Jul 2016 17:55:21 +0100

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.)

>>> +            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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]