hw/net/cadence_gem.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
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);
+ 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
Edgar or Alistair -- could one of you review this
cadence GEM patch, please?
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.
> + 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
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
>
On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@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)?
thanks
-- PMM
On 4/2/25 15:37, Peter Maydell wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@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)?
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, 4 Feb 2025 at 08:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@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
>
On Tue, 4 Feb 2025 at 22:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@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 for your time;
> thanks
> -- PMM
© 2016 - 2026 Red Hat, Inc.