* [PATCH net-next 0/2] net: macb: fix ENST configuration for all queues @ 2026-07-24 4:32 Vineeth Karumanchi 2026-07-24 4:32 ` [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines Vineeth Karumanchi 2026-07-24 4:32 ` [PATCH net-next 2/2] net: macb: configure ENST registers for all queues Vineeth Karumanchi 0 siblings, 2 replies; 11+ messages in thread From: Vineeth Karumanchi @ 2026-07-24 4:32 UTC (permalink / raw) To: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: vineeth.karumanchi, git, netdev, linux-kernel This series fixes the ENST (Enhanced Scheduled Traffic) queue configuration in the macb driver and removes leftover dead defines. When a TAPRIO schedule configured only a subset of queues, the driver left the ENST timing registers of the unconfigured queues at their reset values, resulting in an invalid setup. Patch 2 configures all queues unconditionally, zero-initializing the unconfigured ones, and derives the queue index directly from the gate mask instead of storing a redundant queue_id. Patch 1 removes the fixed per-queue ENST time register defines that are no longer referenced, since the driver computes these addresses from the ENST base offset and the queue index. Vineeth Karumanchi (2): net: macb: remove unused ENST Q0/Q1 time register defines net: macb: configure ENST registers for all queues drivers/net/ethernet/cadence/macb.h | 8 -------- drivers/net/ethernet/cadence/macb_main.c | 20 +++++++++----------- 2 files changed, 9 insertions(+), 19 deletions(-) -- 2.44.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines 2026-07-24 4:32 [PATCH net-next 0/2] net: macb: fix ENST configuration for all queues Vineeth Karumanchi @ 2026-07-24 4:32 ` Vineeth Karumanchi 2026-07-24 17:49 ` Théo Lebrun 2026-07-24 19:40 ` Nicolai Buchwitz 2026-07-24 4:32 ` [PATCH net-next 2/2] net: macb: configure ENST registers for all queues Vineeth Karumanchi 1 sibling, 2 replies; 11+ messages in thread From: Vineeth Karumanchi @ 2026-07-24 4:32 UTC (permalink / raw) To: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: vineeth.karumanchi, git, netdev, linux-kernel The ENST start/on/off time register definitions for Q0 and Q1 are not referenced anywhere in the driver. The driver calculates these register addresses from the ENST base offset and the queue index instead of using fixed defines, removing the unused macros. Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> --- drivers/net/ethernet/cadence/macb.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 2de56017ee0d..a11052565436 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -184,12 +184,6 @@ #define GEM_DCFG8 0x029C /* Design Config 8 */ #define GEM_DCFG10 0x02A4 /* Design Config 10 */ #define GEM_DCFG12 0x02AC /* Design Config 12 */ -#define GEM_ENST_START_TIME_Q0 0x0800 /* ENST Q0 start time */ -#define GEM_ENST_START_TIME_Q1 0x0804 /* ENST Q1 start time */ -#define GEM_ENST_ON_TIME_Q0 0x0820 /* ENST Q0 on time */ -#define GEM_ENST_ON_TIME_Q1 0x0824 /* ENST Q1 on time */ -#define GEM_ENST_OFF_TIME_Q0 0x0840 /* ENST Q0 off time */ -#define GEM_ENST_OFF_TIME_Q1 0x0844 /* ENST Q1 off time */ #define GEM_ENST_CONTROL 0x0880 /* ENST control register */ #define GEM_USX_CONTROL 0x0A80 /* High speed PCS control register */ #define GEM_USX_STATUS 0x0A88 /* High speed PCS status register */ -- 2.44.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines 2026-07-24 4:32 ` [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines Vineeth Karumanchi @ 2026-07-24 17:49 ` Théo Lebrun 2026-07-24 19:40 ` Nicolai Buchwitz 1 sibling, 0 replies; 11+ messages in thread From: Théo Lebrun @ 2026-07-24 17:49 UTC (permalink / raw) To: Vineeth Karumanchi, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: git, netdev, linux-kernel Hello Vineeth, On Fri Jul 24, 2026 at 6:32 AM CEST, Vineeth Karumanchi wrote: > The ENST start/on/off time register definitions for Q0 and Q1 are not > referenced anywhere in the driver. The driver calculates these register > addresses from the ENST base offset and the queue index instead of using > fixed defines, removing the unused macros. > > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > --- > drivers/net/ethernet/cadence/macb.h | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index 2de56017ee0d..a11052565436 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -184,12 +184,6 @@ > #define GEM_DCFG8 0x029C /* Design Config 8 */ > #define GEM_DCFG10 0x02A4 /* Design Config 10 */ > #define GEM_DCFG12 0x02AC /* Design Config 12 */ > -#define GEM_ENST_START_TIME_Q0 0x0800 /* ENST Q0 start time */ > -#define GEM_ENST_START_TIME_Q1 0x0804 /* ENST Q1 start time */ > -#define GEM_ENST_ON_TIME_Q0 0x0820 /* ENST Q0 on time */ > -#define GEM_ENST_ON_TIME_Q1 0x0824 /* ENST Q1 on time */ > -#define GEM_ENST_OFF_TIME_Q0 0x0840 /* ENST Q0 off time */ > -#define GEM_ENST_OFF_TIME_Q1 0x0844 /* ENST Q1 off time */ > #define GEM_ENST_CONTROL 0x0880 /* ENST control register */ > #define GEM_USX_CONTROL 0x0A80 /* High speed PCS control register */ > #define GEM_USX_STATUS 0x0A88 /* High speed PCS status register */ Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com> Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines 2026-07-24 4:32 ` [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines Vineeth Karumanchi 2026-07-24 17:49 ` Théo Lebrun @ 2026-07-24 19:40 ` Nicolai Buchwitz 1 sibling, 0 replies; 11+ messages in thread From: Nicolai Buchwitz @ 2026-07-24 19:40 UTC (permalink / raw) To: Vineeth Karumanchi Cc: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni, git, netdev, linux-kernel Hi Vineeth On 24.7.2026 06:32, Vineeth Karumanchi wrote: > The ENST start/on/off time register definitions for Q0 and Q1 are not > referenced anywhere in the driver. The driver calculates these register > addresses from the ENST base offset and the queue index instead of > using > fixed defines, removing the unused macros. > > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > --- > drivers/net/ethernet/cadence/macb.h | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h > b/drivers/net/ethernet/cadence/macb.h > index 2de56017ee0d..a11052565436 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -184,12 +184,6 @@ > #define GEM_DCFG8 0x029C /* Design Config 8 */ > #define GEM_DCFG10 0x02A4 /* Design Config 10 */ > #define GEM_DCFG12 0x02AC /* Design Config 12 */ > -#define GEM_ENST_START_TIME_Q0 0x0800 /* ENST Q0 start time */ > -#define GEM_ENST_START_TIME_Q1 0x0804 /* ENST Q1 start time */ > -#define GEM_ENST_ON_TIME_Q0 0x0820 /* ENST Q0 on time */ > -#define GEM_ENST_ON_TIME_Q1 0x0824 /* ENST Q1 on time */ > -#define GEM_ENST_OFF_TIME_Q0 0x0840 /* ENST Q0 off time */ > -#define GEM_ENST_OFF_TIME_Q1 0x0844 /* ENST Q1 off time */ > #define GEM_ENST_CONTROL 0x0880 /* ENST control register */ > #define GEM_USX_CONTROL 0x0A80 /* High speed PCS control register */ > #define GEM_USX_STATUS 0x0A88 /* High speed PCS status register */ Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de> Thanks Nicolai ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-24 4:32 [PATCH net-next 0/2] net: macb: fix ENST configuration for all queues Vineeth Karumanchi 2026-07-24 4:32 ` [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines Vineeth Karumanchi @ 2026-07-24 4:32 ` Vineeth Karumanchi 2026-07-24 18:06 ` Théo Lebrun 2026-07-24 19:57 ` Nicolai Buchwitz 1 sibling, 2 replies; 11+ messages in thread From: Vineeth Karumanchi @ 2026-07-24 4:32 UTC (permalink / raw) To: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: vineeth.karumanchi, git, netdev, linux-kernel The initial "tc" command was not overwriting the reset value of ENST registers if only a subset of queues were configured, leading to an invalid setup. To fix this, configure all queues unconditionally. Unconfigured queues are zero-initialized via kcalloc(), ensuring a complete and consistent configuration. The queue_id field in struct macb_queue_enst_config becomes redundant once the array is indexed by queue id, so drop it. Key changes: - Drop queue_id from struct macb_queue_enst_config - Allocate enst_queue[] based on bp->num_queues instead of conf->num_entries - Index enst_queue[] directly with the queue number - Program ENST registers for all queues Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support") Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> --- drivers/net/ethernet/cadence/macb.h | 2 -- drivers/net/ethernet/cadence/macb_main.c | 20 +++++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index a11052565436..4cc9aa9b2f9e 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -1490,7 +1490,6 @@ struct macb_platform_data { * @start_time_mask: Bitmask representing the start time for the queue * @on_time_bytes: "on" time nsec expressed in bytes * @off_time_bytes: "off" time nsec expressed in bytes - * @queue_id: Identifier for the queue * * This structure holds the configuration parameters for an ENST queue, * used to control time-based transmission scheduling in the MACB driver. @@ -1499,7 +1498,6 @@ struct macb_queue_enst_config { u32 start_time_mask; u32 on_time_bytes; u32 off_time_bytes; - u8 queue_id; }; #endif /* _MACB_H */ diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index d394f1f43b68..d58430fe9c41 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4329,7 +4329,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev, struct macb_queue *queue; u32 queue_mask; u8 queue_id; - size_t i; + size_t i, q; int err; if (conf->num_entries > bp->num_queues) { @@ -4357,7 +4357,7 @@ static int macb_taprio_setup_replace(struct net_device *ndev, return -EINVAL; } - enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL); + enst_queue = kcalloc(bp->num_queues, sizeof(*enst_queue), GFP_KERNEL); if (unlikely(!enst_queue)) return -ENOMEM; @@ -4416,13 +4416,12 @@ static int macb_taprio_setup_replace(struct net_device *ndev, goto cleanup; } - enst_queue[i].queue_id = queue_id; - enst_queue[i].start_time_mask = + enst_queue[queue_id].start_time_mask = (start_time_sec << GEM_START_TIME_SEC_OFFSET) | start_time_nsec; - enst_queue[i].on_time_bytes = + enst_queue[queue_id].on_time_bytes = enst_ns_to_hw_units(entry->interval, speed); - enst_queue[i].off_time_bytes = + enst_queue[queue_id].off_time_bytes = enst_ns_to_hw_units(conf->cycle_time - entry->interval, speed); configured_queues |= entry->gate_mask; @@ -4448,15 +4447,14 @@ static int macb_taprio_setup_replace(struct net_device *ndev, gem_writel(bp, ENST_CONTROL, queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET); - for (i = 0; i < conf->num_entries; i++) { - queue = &bp->queues[enst_queue[i].queue_id]; + for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { /* Configure queue timing registers */ queue_writel(queue, ENST_START_TIME, - enst_queue[i].start_time_mask); + enst_queue[q].start_time_mask); queue_writel(queue, ENST_ON_TIME, - enst_queue[i].on_time_bytes); + enst_queue[q].on_time_bytes); queue_writel(queue, ENST_OFF_TIME, - enst_queue[i].off_time_bytes); + enst_queue[q].off_time_bytes); } /* Enable ENST for all configured queues in one write */ -- 2.44.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-24 4:32 ` [PATCH net-next 2/2] net: macb: configure ENST registers for all queues Vineeth Karumanchi @ 2026-07-24 18:06 ` Théo Lebrun 2026-07-27 5:32 ` Karumanchi, Vineeth 2026-07-24 19:57 ` Nicolai Buchwitz 1 sibling, 1 reply; 11+ messages in thread From: Théo Lebrun @ 2026-07-24 18:06 UTC (permalink / raw) To: Vineeth Karumanchi, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: git, netdev, linux-kernel Hello Vineeth, On Fri Jul 24, 2026 at 6:32 AM CEST, Vineeth Karumanchi wrote: > The initial "tc" command was not overwriting the reset value of ENST > registers if only a subset of queues were configured, leading to an > invalid setup. To fix this, configure all queues unconditionally. > Unconfigured queues are zero-initialized via kcalloc(), ensuring a > complete and consistent configuration. But if a subset of queues are configured, the remaining queues don't have their ENST support enabled thanks to ENST_CONTROL and their ENST per-queue register values aren't read? Or HW is broken and reads ENST registers anyway? Or something else I misunderstood? What reset value of ENST regs you observed that caused you trouble? That info could make it into the commit message. > The queue_id field in struct macb_queue_enst_config becomes redundant > once the array is indexed by queue id, so drop it. > > Key changes: > - Drop queue_id from struct macb_queue_enst_config > - Allocate enst_queue[] based on bp->num_queues instead of > conf->num_entries > - Index enst_queue[] directly with the queue number > - Program ENST registers for all queues > > Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support") > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-24 18:06 ` Théo Lebrun @ 2026-07-27 5:32 ` Karumanchi, Vineeth 2026-07-27 12:05 ` Théo Lebrun 0 siblings, 1 reply; 11+ messages in thread From: Karumanchi, Vineeth @ 2026-07-27 5:32 UTC (permalink / raw) To: Théo Lebrun, Vineeth Karumanchi, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: git, netdev, linux-kernel Hi Théo Lebrun, On 7/24/2026 11:36 PM, Théo Lebrun wrote: > Hello Vineeth, > > On Fri Jul 24, 2026 at 6:32 AM CEST, Vineeth Karumanchi wrote: >> The initial "tc" command was not overwriting the reset value of ENST >> registers if only a subset of queues were configured, leading to an >> invalid setup. To fix this, configure all queues unconditionally. >> Unconfigured queues are zero-initialized via kcalloc(), ensuring a >> complete and consistent configuration. > But if a subset of queues are configured, the remaining queues don't > have their ENST support enabled thanks to ENST_CONTROL and their ENST > per-queue register values aren't read? Or HW is broken and reads ENST > registers anyway? Or something else I misunderstood? > > What reset value of ENST regs you observed that caused you trouble? > That info could make it into the commit message. // Yes, this is a confirmed hardware issue. We raised it with Cadence, and they have acknowledged the problem. The reset value of the |enst_on_time_qX| registers is *0x0001FFFF*. During ENST initialization, packet interleaving was observed when only a subset of the available queues was configured, while the remaining queues—whose corresponding |enst_on_time_qX| registers still contained non-zero values—were left disabled in |ENST_CONTROL|. For example, in a configuration where only two of the four queues are enabled, some packets from *Q0* getting transmitted during the *Q1* time slot, and vice versa. Furthermore, once the hardware enters this state, it does not recover even after a complete ENST queue reconfiguration is performed. Thanks 🙏 Vineeth >> The queue_id field in struct macb_queue_enst_config becomes redundant >> once the array is indexed by queue id, so drop it. >> >> Key changes: >> - Drop queue_id from struct macb_queue_enst_config >> - Allocate enst_queue[] based on bp->num_queues instead of >> conf->num_entries >> - Index enst_queue[] directly with the queue number >> - Program ENST registers for all queues >> >> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support") >> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-27 5:32 ` Karumanchi, Vineeth @ 2026-07-27 12:05 ` Théo Lebrun 2026-07-27 12:52 ` Théo Lebrun 0 siblings, 1 reply; 11+ messages in thread From: Théo Lebrun @ 2026-07-27 12:05 UTC (permalink / raw) To: Karumanchi, Vineeth, Vineeth Karumanchi, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: git, netdev, linux-kernel On Mon Jul 27, 2026 at 7:32 AM CEST, Karumanchi, Vineeth wrote: > On 7/24/2026 11:36 PM, Théo Lebrun wrote: >> On Fri Jul 24, 2026 at 6:32 AM CEST, Vineeth Karumanchi wrote: >>> The initial "tc" command was not overwriting the reset value of ENST >>> registers if only a subset of queues were configured, leading to an >>> invalid setup. To fix this, configure all queues unconditionally. >>> Unconfigured queues are zero-initialized via kcalloc(), ensuring a >>> complete and consistent configuration. >> But if a subset of queues are configured, the remaining queues don't >> have their ENST support enabled thanks to ENST_CONTROL and their ENST >> per-queue register values aren't read? Or HW is broken and reads ENST >> registers anyway? Or something else I misunderstood? >> >> What reset value of ENST regs you observed that caused you trouble? >> That info could make it into the commit message. > // > Yes, this is a confirmed hardware issue. We raised it with Cadence, and > they have > acknowledged the problem. The reset value of the |enst_on_time_qX| > registers is *0x0001FFFF*. ACK. 0x1FFFF translates to the max value of on_time. Important also to note that off_time reset value is 0x0 according to the manual. This combination probably explains why queues feel free to emit whenever they want. > During ENST initialization, packet interleaving was observed when only a > subset of the available queues was configured, while the remaining > queues—whose > corresponding |enst_on_time_qX| registers still contained non-zero > values—were left disabled in |ENST_CONTROL|. > > For example, in a configuration where only two of the four queues are > enabled, > some packets from *Q0* getting transmitted during the *Q1* time slot, > and vice versa. This description isn't enough to fully show there is a bug. - You say "only two of the four queues are enabled" but did you mean "queue enabled" or "EnST is enabled on that queue"? - EnST enabled doesn't mean the timeslots don't overlap, this is one allowed config. Maybe it's not a bug? What behavior would you expect when EnST is enabled on some queues only? When should queues without EnST active transmit their frames? Please be exhaustive in your future commit message; thanks! > Furthermore, once the hardware enters this state, it does not recover > even after a > complete ENST queue reconfiguration is performed. --- As you contribute to EnST support, you might be interested in a bug I just noticed in the enst_ns_to_hw_units() implementation. It doesn't do rounding properly. Eg ns=100 speed_mbps=100 => 2 units of time but that is 2*80ns=160ns, whereas one unit of time (80ns) would have been a better choice. This example is the worst case scenario (the closer we are to 1 unit of time, the worst the bug is). --- PS: please note your email formatting is off because of hard wrapping. It's well visible on lore. Email is still readable but less than optimal. https://lore.kernel.org/netdev/3924babc-9755-46ce-97b2-bd6dfce6f6c6@amd.com/ Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-27 12:05 ` Théo Lebrun @ 2026-07-27 12:52 ` Théo Lebrun 0 siblings, 0 replies; 11+ messages in thread From: Théo Lebrun @ 2026-07-27 12:52 UTC (permalink / raw) To: Karumanchi, Vineeth, Vineeth Karumanchi, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni Cc: git, netdev, linux-kernel On Mon Jul 27, 2026 at 2:05 PM CEST, Théo Lebrun wrote: > On Mon Jul 27, 2026 at 7:32 AM CEST, Karumanchi, Vineeth wrote: >> On 7/24/2026 11:36 PM, Théo Lebrun wrote: >>> On Fri Jul 24, 2026 at 6:32 AM CEST, Vineeth Karumanchi wrote: >>>> The initial "tc" command was not overwriting the reset value of ENST >>>> registers if only a subset of queues were configured, leading to an >>>> invalid setup. To fix this, configure all queues unconditionally. >>>> Unconfigured queues are zero-initialized via kcalloc(), ensuring a >>>> complete and consistent configuration. >>> But if a subset of queues are configured, the remaining queues don't >>> have their ENST support enabled thanks to ENST_CONTROL and their ENST >>> per-queue register values aren't read? Or HW is broken and reads ENST >>> registers anyway? Or something else I misunderstood? >>> >>> What reset value of ENST regs you observed that caused you trouble? >>> That info could make it into the commit message. >> // >> Yes, this is a confirmed hardware issue. We raised it with Cadence, and >> they have >> acknowledged the problem. The reset value of the |enst_on_time_qX| >> registers is *0x0001FFFF*. > > ACK. 0x1FFFF translates to the max value of on_time. Important also to > note that off_time reset value is 0x0 according to the manual. This > combination probably explains why queues feel free to emit whenever > they want. > >> During ENST initialization, packet interleaving was observed when only a >> subset of the available queues was configured, while the remaining >> queues—whose >> corresponding |enst_on_time_qX| registers still contained non-zero >> values—were left disabled in |ENST_CONTROL|. >> >> For example, in a configuration where only two of the four queues are >> enabled, >> some packets from *Q0* getting transmitted during the *Q1* time slot, >> and vice versa. > > This description isn't enough to fully show there is a bug. > - You say "only two of the four queues are enabled" but did you mean > "queue enabled" or "EnST is enabled on that queue"? > - EnST enabled doesn't mean the timeslots don't overlap, this is one > allowed config. > > Maybe it's not a bug? What behavior would you expect when EnST is > enabled on some queues only? When should queues without EnST active > transmit their frames? > > Please be exhaustive in your future commit message; thanks! > >> Furthermore, once the hardware enters this state, it does not recover >> even after a >> complete ENST queue reconfiguration is performed. > > --- > > As you contribute to EnST support, you might be interested in a bug I > just noticed in the enst_ns_to_hw_units() implementation. It doesn't do > rounding properly. > > Eg ns=100 speed_mbps=100 => 2 units of time but that is 2*80ns=160ns, > whereas one unit of time (80ns) would have been a better choice. This > example is the worst case scenario (the closer we are to 1 unit of > time, the worst the bug is). Let me backpaddle on this. Maybe it makes sense that we always configure strictly longer timeslot durations to what was asked by userspace. But I'm not convinced. We round up both on_time and off_time for all queues, meaning it's hard for userspace to configure mutually exclusive timeslots. We have no equivalent to clk_ops::determine_rate() for userspace to query what values we support. An LLM reported to me 5 out of 7 drivers write nanoseconds values directly. macb and am65-cpsw are the two exceptions and we use the same formula: DIV_ROUND_UP(ns*speed, 8000). > --- > > PS: please note your email formatting is off because of hard wrapping. > It's well visible on lore. Email is still readable but less than optimal. > > https://lore.kernel.org/netdev/3924babc-9755-46ce-97b2-bd6dfce6f6c6@amd.com/ > > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-24 4:32 ` [PATCH net-next 2/2] net: macb: configure ENST registers for all queues Vineeth Karumanchi 2026-07-24 18:06 ` Théo Lebrun @ 2026-07-24 19:57 ` Nicolai Buchwitz 2026-07-27 10:01 ` Karumanchi, Vineeth 1 sibling, 1 reply; 11+ messages in thread From: Nicolai Buchwitz @ 2026-07-24 19:57 UTC (permalink / raw) To: Vineeth Karumanchi Cc: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni, git, netdev, linux-kernel Hi Vineeth On 24.7.2026 06:32, Vineeth Karumanchi wrote: > The initial "tc" command was not overwriting the reset value of ENST > registers if only a subset of queues were configured, leading to an > invalid setup. To fix this, configure all queues unconditionally. > Unconfigured queues are zero-initialized via kcalloc(), ensuring a > complete and consistent configuration. > > The queue_id field in struct macb_queue_enst_config becomes redundant > once the array is indexed by queue id, so drop it. > > Key changes: > - Drop queue_id from struct macb_queue_enst_config > - Allocate enst_queue[] based on bp->num_queues instead of > conf->num_entries > - Index enst_queue[] directly with the queue number > - Program ENST registers for all queues > > Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling > support") In addition to what Théo already mentioned: The series targets net-next, but if this is a real bug fix it should probably be routed via net. The offending commit is in stable since v6.18. > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > --- > drivers/net/ethernet/cadence/macb.h | 2 -- > drivers/net/ethernet/cadence/macb_main.c | 20 +++++++++----------- > 2 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h > b/drivers/net/ethernet/cadence/macb.h > index a11052565436..4cc9aa9b2f9e 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -1490,7 +1490,6 @@ struct macb_platform_data { > * @start_time_mask: Bitmask representing the start time for the > queue > * @on_time_bytes: "on" time nsec expressed in bytes > * @off_time_bytes: "off" time nsec expressed in bytes > - * @queue_id: Identifier for the queue > * > * This structure holds the configuration parameters for an ENST > queue, > * used to control time-based transmission scheduling in the MACB > driver. > @@ -1499,7 +1498,6 @@ struct macb_queue_enst_config { > u32 start_time_mask; > u32 on_time_bytes; > u32 off_time_bytes; > - u8 queue_id; > }; > > #endif /* _MACB_H */ > diff --git a/drivers/net/ethernet/cadence/macb_main.c > b/drivers/net/ethernet/cadence/macb_main.c > index d394f1f43b68..d58430fe9c41 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -4329,7 +4329,7 @@ static int macb_taprio_setup_replace(struct > net_device *ndev, > struct macb_queue *queue; > u32 queue_mask; > u8 queue_id; > - size_t i; > + size_t i, q; > int err; > > if (conf->num_entries > bp->num_queues) { > @@ -4357,7 +4357,7 @@ static int macb_taprio_setup_replace(struct > net_device *ndev, > return -EINVAL; > } > > - enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), > GFP_KERNEL); > + enst_queue = kcalloc(bp->num_queues, sizeof(*enst_queue), > GFP_KERNEL); > if (unlikely(!enst_queue)) > return -ENOMEM; > > @@ -4416,13 +4416,12 @@ static int macb_taprio_setup_replace(struct > net_device *ndev, > goto cleanup; > } > > - enst_queue[i].queue_id = queue_id; > - enst_queue[i].start_time_mask = > + enst_queue[queue_id].start_time_mask = > (start_time_sec << GEM_START_TIME_SEC_OFFSET) | > start_time_nsec; > - enst_queue[i].on_time_bytes = > + enst_queue[queue_id].on_time_bytes = > enst_ns_to_hw_units(entry->interval, speed); > - enst_queue[i].off_time_bytes = > + enst_queue[queue_id].off_time_bytes = > enst_ns_to_hw_units(conf->cycle_time - entry->interval, speed); > > configured_queues |= entry->gate_mask; > @@ -4448,15 +4447,14 @@ static int macb_taprio_setup_replace(struct > net_device *ndev, > gem_writel(bp, ENST_CONTROL, > queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET); > > - for (i = 0; i < conf->num_entries; i++) { > - queue = &bp->queues[enst_queue[i].queue_id]; > + for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) { > /* Configure queue timing registers */ > queue_writel(queue, ENST_START_TIME, > - enst_queue[i].start_time_mask); > + enst_queue[q].start_time_mask); > queue_writel(queue, ENST_ON_TIME, > - enst_queue[i].on_time_bytes); > + enst_queue[q].on_time_bytes); > queue_writel(queue, ENST_OFF_TIME, > - enst_queue[i].off_time_bytes); > + enst_queue[q].off_time_bytes); > } > > /* Enable ENST for all configured queues in one write */ Regards, Nicolai ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] net: macb: configure ENST registers for all queues 2026-07-24 19:57 ` Nicolai Buchwitz @ 2026-07-27 10:01 ` Karumanchi, Vineeth 0 siblings, 0 replies; 11+ messages in thread From: Karumanchi, Vineeth @ 2026-07-27 10:01 UTC (permalink / raw) To: Nicolai Buchwitz, Vineeth Karumanchi Cc: theo.lebrun, conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni, git, netdev, linux-kernel Hi Nicolai, On 7/25/2026 1:27 AM, Nicolai Buchwitz wrote: <..> >> Key changes: >> - Drop queue_id from struct macb_queue_enst_config >> - Allocate enst_queue[] based on bp->num_queues instead of >> conf->num_entries >> - Index enst_queue[] directly with the queue number >> - Program ENST registers for all queues >> >> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support") > > In addition to what Théo already mentioned: > > The series targets net-next, but if this is a real bug fix it should > probably be routed via net. The offending commit is in stable since v6.18. Sure, I will split the patch series accordingly and submit the updated versions to *net-next* and *net*. Thanks -- 🙏 Vineeth ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-07-27 12:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-24 4:32 [PATCH net-next 0/2] net: macb: fix ENST configuration for all queues Vineeth Karumanchi 2026-07-24 4:32 ` [PATCH net-next 1/2] net: macb: remove unused ENST Q0/Q1 time register defines Vineeth Karumanchi 2026-07-24 17:49 ` Théo Lebrun 2026-07-24 19:40 ` Nicolai Buchwitz 2026-07-24 4:32 ` [PATCH net-next 2/2] net: macb: configure ENST registers for all queues Vineeth Karumanchi 2026-07-24 18:06 ` Théo Lebrun 2026-07-27 5:32 ` Karumanchi, Vineeth 2026-07-27 12:05 ` Théo Lebrun 2026-07-27 12:52 ` Théo Lebrun 2026-07-24 19:57 ` Nicolai Buchwitz 2026-07-27 10:01 ` Karumanchi, Vineeth
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox