Re: [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion

["claudiu beznea (tuxon)" <claudiu.beznea@tuxon.dev>]

Hi, Vineeth,

On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Add Enhanced Network Scheduling and Timing (ENST) support to
> queue infrastructure with speed-dependent timing calculations for
> precise gate control.
> 
> Hardware timing unit conversion:
> - Timing values programmed as hardware units based on link speed
> - Conversion formula: time_bytes = time_ns / divisor
> - Speed-specific divisors:
>    * 1 Gbps:   divisor = 8
>    * 100 Mbps: divisor = 80
>    * 10 Mbps:  divisor = 800
> 
> Infrastructure changes:
> - Extend macb_queue structure with ENST timing control registers
> - Add queue_enst_configs structure for per-entry TC configuration storage
> - Map ENST register offsets into existing queue management framework
> - Define ENST_NS_TO_HW_UNITS() macro for automatic speed-based conversion
> 
> This enables hardware-native timing programming while abstracting the
> speed-dependent conversions
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb.h      | 32 ++++++++++++++++++++++++
>   drivers/net/ethernet/cadence/macb_main.c |  6 +++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index e456ac65d6c6..ef3995564c5c 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -857,6 +857,16 @@
>   
>   #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
>   
> +/* ENST macros*/
> +#define ENST_NS_TO_HW_UNITS(ns, speed_mbps) \
> +		DIV_ROUND_UP((ns) * (speed_mbps), (ENST_TIME_GRANULARITY_NS * 1000))
> +
> +#define ENST_MAX_HW_INTERVAL(speed_mbps) \
> +		DIV_ROUND_UP(GENMASK(GEM_ON_TIME_SIZE - 1, 0) * ENST_TIME_GRANULARITY_NS * 1000,\
> +		(speed_mbps))
> +
> +#define ENST_MAX_START_TIME_SEC GENMASK(GEM_START_TIME_SEC_SIZE - 1, 0)

These are not used in this patch.

> +
>   /* struct macb_dma_desc - Hardware DMA descriptor
>    * @addr: DMA address of data buffer
>    * @ctrl: Control and status bits
> @@ -1262,6 +1272,11 @@ struct macb_queue {
>   	unsigned int		RBQP;
>   	unsigned int		RBQPH;
>   
> +	/* ENST register offsets for this queue */
> +	unsigned int		ENST_START_TIME;
> +	unsigned int		ENST_ON_TIME;
> +	unsigned int		ENST_OFF_TIME;
> +
>   	/* Lock to protect tx_head and tx_tail */
>   	spinlock_t		tx_ptr_lock;
>   	unsigned int		tx_head, tx_tail;
> @@ -1450,4 +1465,21 @@ struct macb_platform_data {
>   	struct clk	*hclk;
>   };
>   
> +/**
> + * struct queue_enst_configs - Configuration for Enhanced Scheduled Traffic (ENST) queue
> + * @queue_id:         Identifier for the queue
> + * @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
> + *
> + * This structure holds the configuration parameters for an ENST queue,
> + * used to control time-based transmission scheduling in the MACB driver.
> + */
> +struct queue_enst_configs {

s/queue_enst_config/macb_queue_enst_config

> +	u8 queue_id;

Could you please move this above to avoid any padding?

> +	u32 start_time_mask;
> +	u32 on_time_bytes;
> +	u32 off_time_bytes;
> +};
> +

This structure is not used in this patch. Can you please introduce it in the 
patch it is used?


>   #endif /* _MACB_H */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ce95fad8cedd..ff87d3e1d8a0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4305,6 +4305,9 @@ static int macb_init(struct platform_device *pdev)
>   			queue->TBQP = GEM_TBQP(hw_q - 1);
>   			queue->RBQP = GEM_RBQP(hw_q - 1);
>   			queue->RBQS = GEM_RBQS(hw_q - 1);
> +			queue->ENST_START_TIME = GEM_ENST_START_TIME(hw_q);
> +			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(hw_q);
> +			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(hw_q);

You can drop these lines here and move it outside of the if (hw_q) {} else {} block.

>   #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>   			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>   				queue->TBQPH = GEM_TBQPH(hw_q - 1);
> @@ -4319,6 +4322,9 @@ static int macb_init(struct platform_device *pdev)
>   			queue->IMR  = MACB_IMR;
>   			queue->TBQP = MACB_TBQP;
>   			queue->RBQP = MACB_RBQP;
> +			queue->ENST_START_TIME = GEM_ENST_START_TIME(0);
> +			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(0);
> +			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(0);

With the above suggested change, these lines could be dropped.

On the other hand these lines are used in patch 3. So I would prefer to have 
them introduced there.

Thank you,
Claudiu

>   #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>   			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>   				queue->TBQPH = MACB_TBQPH;