| Message ID | 1588935645-20351-8-git-send-email-sai.pavan.boddu@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Cadence GEM Fixes | expand |
On Fri, May 08, 2020 at 04:30:41PM +0530, Sai Pavan Boddu wrote: > Add a property "jumbo-max-len", which can be configured for jumbo frame size > up to 16,383 bytes, and also introduce new register GEM_JUMBO_MAX_LEN. > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> > --- > hw/net/cadence_gem.c | 21 +++++++++++++++++++-- > include/hw/net/cadence_gem.h | 1 + > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index 5ccec1a..45c50ab 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -61,6 +61,7 @@ > #define GEM_TXPAUSE (0x0000003C/4) /* TX Pause Time reg */ > #define GEM_TXPARTIALSF (0x00000040/4) /* TX Partial Store and Forward */ > #define GEM_RXPARTIALSF (0x00000044/4) /* RX Partial Store and Forward */ > +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame Size */ Would be nice to align this in the same way as all the others... > #define GEM_HASHLO (0x00000080/4) /* Hash Low address reg */ > #define GEM_HASHHI (0x00000084/4) /* Hash High address reg */ > #define GEM_SPADDR1LO (0x00000088/4) /* Specific addr 1 low reg */ > @@ -314,7 +315,8 @@ > > #define GEM_MODID_VALUE 0x00020118 > > -#define MAX_FRAME_SIZE 2048 > +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF > +#define MAX_FRAME_SIZE MAX_JUMBO_FRAME_SIZE_MASK > > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc) > { > @@ -1343,9 +1345,10 @@ static void gem_reset(DeviceState *d) > s->regs[GEM_RXPARTIALSF] = 0x000003ff; > s->regs[GEM_MODID] = s->revision; > s->regs[GEM_DESCONF] = 0x02500111; > - s->regs[GEM_DESCONF2] = 0x2ab13fff; > + s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len; > s->regs[GEM_DESCONF5] = 0x002f2045; > s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK; > + s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len; > > if (s->num_priority_queues > 1) { > queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1); > @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size) > DB_PRINT("lowering irqs on ISR read\n"); > /* The interrupts get updated at the end of the function. */ > break; > + case GEM_JUMBO_MAX_LEN: > + retval = s->jumbo_max_len; > + break; > case GEM_PHYMNTNC: > if (retval & GEM_PHYMNTNC_OP_R) { > uint32_t phy_addr, reg_num; > @@ -1516,6 +1522,9 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val, > s->regs[GEM_IMR] &= ~val; > gem_update_int_status(s); > break; > + case GEM_JUMBO_MAX_LEN: > + s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK; I don't think writing to this register may increase the max len beyond the max-len selected at design time (the property). TBH I'm surprised this register is RW in the spec. We may need two variables here, one for the design-time configured max and another for the runtime configurable max. > + break; > case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE: > s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val; > gem_update_int_status(s); > @@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error **errp) > s->nic = qemu_new_nic(&net_gem_info, &s->conf, > object_get_typename(OBJECT(dev)), dev->id, s); > > + if (s->jumbo_max_len > MAX_FRAME_SIZE) { > + g_warning("jumbo-max-len is grater than %d", You've got a typo here "grater". I also think we could error out here if wrong values are chosen. Best regards, Edgar
Hi Edgar, > -----Original Message----- > From: Edgar E. Iglesias <edgar.iglesias@gmail.com> > Sent: Friday, May 8, 2020 5:14 PM > To: Sai Pavan Boddu <saipava@xilinx.com> > Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell > <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus > Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé > <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried > <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu- > devel@nongnu.org > Subject: Re: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo > frames > > On Fri, May 08, 2020 at 04:30:41PM +0530, Sai Pavan Boddu wrote: > > Add a property "jumbo-max-len", which can be configured for jumbo > > frame size up to 16,383 bytes, and also introduce new register > GEM_JUMBO_MAX_LEN. > > > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> > > --- > > hw/net/cadence_gem.c | 21 +++++++++++++++++++-- > > include/hw/net/cadence_gem.h | 1 + > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > > 5ccec1a..45c50ab 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -61,6 +61,7 @@ > > #define GEM_TXPAUSE (0x0000003C/4) /* TX Pause Time reg */ > > #define GEM_TXPARTIALSF (0x00000040/4) /* TX Partial Store and > Forward */ > > #define GEM_RXPARTIALSF (0x00000044/4) /* RX Partial Store and > Forward */ > > +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame > Size */ > > Would be nice to align this in the same way as all the others... > > > > > #define GEM_HASHLO (0x00000080/4) /* Hash Low address reg */ > > #define GEM_HASHHI (0x00000084/4) /* Hash High address reg */ > > #define GEM_SPADDR1LO (0x00000088/4) /* Specific addr 1 low reg */ > > @@ -314,7 +315,8 @@ > > > > #define GEM_MODID_VALUE 0x00020118 > > > > -#define MAX_FRAME_SIZE 2048 > > +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF #define > MAX_FRAME_SIZE > > +MAX_JUMBO_FRAME_SIZE_MASK > > > > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, > > uint32_t *desc) { @@ -1343,9 +1345,10 @@ static void > > gem_reset(DeviceState *d) > > s->regs[GEM_RXPARTIALSF] = 0x000003ff; > > s->regs[GEM_MODID] = s->revision; > > s->regs[GEM_DESCONF] = 0x02500111; > > - s->regs[GEM_DESCONF2] = 0x2ab13fff; > > + s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len; > > s->regs[GEM_DESCONF5] = 0x002f2045; > > s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK; > > + s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len; > > > > if (s->num_priority_queues > 1) { > > queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1); > > @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr > offset, unsigned size) > > DB_PRINT("lowering irqs on ISR read\n"); > > /* The interrupts get updated at the end of the function. */ > > break; > > + case GEM_JUMBO_MAX_LEN: > > + retval = s->jumbo_max_len; > > + break; > > case GEM_PHYMNTNC: > > if (retval & GEM_PHYMNTNC_OP_R) { > > uint32_t phy_addr, reg_num; @@ -1516,6 +1522,9 @@ static > > void gem_write(void *opaque, hwaddr offset, uint64_t val, > > s->regs[GEM_IMR] &= ~val; > > gem_update_int_status(s); > > break; > > + case GEM_JUMBO_MAX_LEN: > > + s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK; > > I don't think writing to this register may increase the max len beyond the > max-len selected at design time (the property). > TBH I'm surprised this register is RW in the spec. > > We may need two variables here, one for the design-time configured max > and another for the runtime configurable max. [Sai Pavan Boddu] Better way is to, use new created property to set the reset value of this register. Which can be overwritten by guest on runtime by writing to regs[GEM_JUMBO_MAX_LEN]. I would add few checks, so that frames does not cross JUMBO max length configured. Thanks, Sai Pavan > > > > + break; > > case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE: > > s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= > ~val; > > gem_update_int_status(s); > > @@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error > **errp) > > s->nic = qemu_new_nic(&net_gem_info, &s->conf, > > object_get_typename(OBJECT(dev)), dev->id, > > s); > > > > + if (s->jumbo_max_len > MAX_FRAME_SIZE) { > > + g_warning("jumbo-max-len is grater than %d", > > > You've got a typo here "grater". > > I also think we could error out here if wrong values are chosen. > > Best regards, > Edgar
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 5ccec1a..45c50ab 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -61,6 +61,7 @@ #define GEM_TXPAUSE (0x0000003C/4) /* TX Pause Time reg */ #define GEM_TXPARTIALSF (0x00000040/4) /* TX Partial Store and Forward */ #define GEM_RXPARTIALSF (0x00000044/4) /* RX Partial Store and Forward */ +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame Size */ #define GEM_HASHLO (0x00000080/4) /* Hash Low address reg */ #define GEM_HASHHI (0x00000084/4) /* Hash High address reg */ #define GEM_SPADDR1LO (0x00000088/4) /* Specific addr 1 low reg */ @@ -314,7 +315,8 @@ #define GEM_MODID_VALUE 0x00020118 -#define MAX_FRAME_SIZE 2048 +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF +#define MAX_FRAME_SIZE MAX_JUMBO_FRAME_SIZE_MASK static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc) { @@ -1343,9 +1345,10 @@ static void gem_reset(DeviceState *d) s->regs[GEM_RXPARTIALSF] = 0x000003ff; s->regs[GEM_MODID] = s->revision; s->regs[GEM_DESCONF] = 0x02500111; - s->regs[GEM_DESCONF2] = 0x2ab13fff; + s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len; s->regs[GEM_DESCONF5] = 0x002f2045; s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK; + s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len; if (s->num_priority_queues > 1) { queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1); @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size) DB_PRINT("lowering irqs on ISR read\n"); /* The interrupts get updated at the end of the function. */ break; + case GEM_JUMBO_MAX_LEN: + retval = s->jumbo_max_len; + break; case GEM_PHYMNTNC: if (retval & GEM_PHYMNTNC_OP_R) { uint32_t phy_addr, reg_num; @@ -1516,6 +1522,9 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val, s->regs[GEM_IMR] &= ~val; gem_update_int_status(s); break; + case GEM_JUMBO_MAX_LEN: + s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK; + break; case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE: s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val; gem_update_int_status(s); @@ -1611,6 +1620,12 @@ static void gem_realize(DeviceState *dev, Error **errp) s->nic = qemu_new_nic(&net_gem_info, &s->conf, object_get_typename(OBJECT(dev)), dev->id, s); + if (s->jumbo_max_len > MAX_FRAME_SIZE) { + g_warning("jumbo-max-len is grater than %d", + MAX_FRAME_SIZE); + s->jumbo_max_len = MAX_FRAME_SIZE; + } + s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE); s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE); } @@ -1670,6 +1685,8 @@ static Property gem_properties[] = { num_type1_screeners, 4), DEFINE_PROP_UINT8("num-type2-screeners", CadenceGEMState, num_type2_screeners, 4), + DEFINE_PROP_UINT16("jumbo-max-len", CadenceGEMState, + jumbo_max_len, 10240), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h index 8dbbaa3..ef85737 100644 --- a/include/hw/net/cadence_gem.h +++ b/include/hw/net/cadence_gem.h @@ -82,6 +82,7 @@ typedef struct CadenceGEMState { uint8_t *tx_packet; uint8_t *rx_packet; + uint16_t jumbo_max_len; uint32_t rx_desc[MAX_PRIORITY_QUEUES][DESC_MAX_NUM_WORDS]; bool sar_active[4];
Add a property "jumbo-max-len", which can be configured for jumbo frame size up to 16,383 bytes, and also introduce new register GEM_JUMBO_MAX_LEN. Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> --- hw/net/cadence_gem.c | 21 +++++++++++++++++++-- include/hw/net/cadence_gem.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-)