Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]

On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Edgar or Alistair -- could one of you review this
> cadence GEM patch, please?
>
>
Sorry for the delay!



> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com>
> wrote:
> >
> > From: Andrew Yuan <andrew.yuan@jaguarmicro.com>
> >
> > As in the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP
> Rev: R1p12 - Doc Rev: 1.3 User Guide,
> > if the DISABLE_MASK bit in type2_compare_x_word_1 is set,
> > mask_value in type2_compare_x_word_0 is used as an additional 2 byte
> Compare Value
> >
> > Signed-off-by: Andrew Yuan <andrew.yuan@jaguarmicro.com>
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/net/cadence_gem.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index 3fce01315f..7bd176951e 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -909,8 +909,8 @@ static int get_queue_from_screen(CadenceGEMState *s,
> uint8_t *rxbuf_ptr,
> >
> >          /* Compare A, B, C */
> >          for (j = 0; j < 3; j++) {
> > -            uint32_t cr0, cr1, mask, compare;
> > -            uint16_t rx_cmp;
> > +            uint32_t cr0, cr1, mask, compare, disable_mask;
> > +            uint32_t rx_cmp;
> >              int offset;
> >              int cr_idx = extract32(reg,
> R_SCREENING_TYPE2_REG0_COMPARE_A_SHIFT + j * 6,
> >
>  R_SCREENING_TYPE2_REG0_COMPARE_A_LENGTH);
> > @@ -946,9 +946,23 @@ static int get_queue_from_screen(CadenceGEMState
> *s, uint8_t *rxbuf_ptr,
> >                  break;
> >              }
> >
> > -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> > -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
> > -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> COMPARE_VALUE);
> > +            disable_mask =
> > +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
> > +            if (disable_mask) {
> > +                /*
> > +                 * If disable_mask is set,
> > +                 * mask_value is used as an additional 2 byte Compare
> Value.
> > +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is
> set.
> > +                 */
> > +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
> > +                mask = 0xFFFFFFFF;
> > +                compare = cr0;
> > +            } else {
> > +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
>
> Is the change in behaviour in the !disable_mask codepath here
> intentional? Previously we use one byte from rxbuf_ptr[offset],
> duplicated into both halves of rx_cmp; now we will load two
> different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
>
> If this is intended, we should say so in the commit message.
>
>
I agree that it should be mentioned (looks like a correct bugfix).
Other than that this patch looks good to me!

Cheers,
Edgar


>
> > +                mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> MASK_VALUE);
> > +                compare =
> > +                    FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> COMPARE_VALUE);
> > +            }
> >
> >              if ((rx_cmp & mask) == (compare & mask)) {
> >                  matched = true;
> > --
> > 2.25.1
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 5056 bytes --]