On Tue, 4 Feb 2025 at 08:37, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
> <edgar.igles...@gmail.com> wrote:
> > On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.mayd...@linaro.org>
> wrote:
> >> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.y...@jaguarmicro.com>
> wrote:
> >> > - 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).
>
> Thanks. I've expanded the commit message:
>
> hw/net/cadence_gem: Fix the mask/compare/disable-mask logic
>
> Our current handling of the mask/compare logic in the Cadence
> GEM ethernet device is wrong:
> (1) we load the same byte twice from rx_buf when
> creating the compare value
> (2) we ignore the DISABLE_MASK flag
>
> The "Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev:
> R1p12 - Doc Rev: 1.3 User Guide" states that if the DISABLE_MASK bit
> in type2_compare_x_word_1 is set, the mask_value field in
> type2_compare_x_word_0 is used as an additional 2 byte Compare Value.
>
> Correct these bugs:
> * in the !disable_mask codepath, use lduw_le_p() so we
> correctly load a 16-bit value for comparison
> * in the disable_mask codepath, we load a full 4-byte value
> from rx_buf for the comparison, set the compare value to
> the whole of the cr0 register (i.e. the concatenation of
> the mask and compare fields), and set mask to 0xffffffff
> to force a 32-bit comparison
>
> and also tweaked the comment a bit:
>
> + /*
> + * If disable_mask is set, mask_value is used as an
> + * additional 2 byte Compare Value; that is equivalent
> + * to using the whole cr0 register as the comparison
> value.
> + * Load 32 bits of data from rx_buf, and set mask to
> + * all-ones so we compare all 32 bits.
> + */
>
> and applied this to target-arm.next.
>
> > Other than that this patch looks good to me!
>
> Can I call that a Reviewed-by (with the above changes)?
Yes, thanks!!
>
> thanks
> -- PMM
>
