diff mbox series

net: stmmac: fix rx queue priority assignment

Message ID 20240219102405.32015-1-piotrwejman90@gmail.com
State New
Headers
Series net: stmmac: fix rx queue priority assignment |

Commit Message

Piotr Wejman Feb. 19, 2024, 10:24 a.m. UTC 
  The driver should ensure that same priority is not mapped to multiple
rx queues. Currently dwmac4_rx_queue_priority function is adding
priorities for a queue without clearing them from others.

From DesignWare Cores Ethernet Quality-of-Service
Databook, section 17.1.29 MAC_RxQ_Ctrl2:
"[...]The software must ensure that the content of this field is
mutually exclusive to the PSRQ fields for other queues, that is,
the same priority is not mapped to multiple Rx queues[...]"

After this patch, dwmac4_rx_queue_priority function will:
- assign desired priorities to a queue
- remove those priorities from all other queues
The write sequence of CTRL2 and CTRL3 registers is done in the way to
ensure this order.

Also, the PSRQn field contains the mask of priorities and not only one
priority. Rename "prio" argument to "prio_mask".

Signed-off-by: Piotr Wejman <piotrwejman90@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 +++++++++++++------
 1 file changed, 26 insertions(+), 10 deletions(-)

Comments

Romain Gantois Feb. 19, 2024, 10:40 a.m. UTC | #1 
Hello Piotr,

On Mon, 19 Feb 2024, Piotr Wejman wrote:

>  static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
> -				     u32 prio, u32 queue)
> +				     u32 prio_mask, u32 queue)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
> -	u32 base_register;
> -	u32 value;
> +	u32 clear_mask = 0;
> +	u32 ctrl2, ctrl3;
> +	int i;
>  
> -	base_register = (queue < 4) ? GMAC_RXQ_CTRL2 : GMAC_RXQ_CTRL3;
> -	if (queue >= 4)
> -		queue -= 4;
> +	ctrl2 = readl(ioaddr + GMAC_RXQ_CTRL2);
> +	ctrl3 = readl(ioaddr + GMAC_RXQ_CTRL3);
>  
> -	value = readl(ioaddr + base_register);
> +	for (i = 0; i < 4; i++)
> +		clear_mask |= ((prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(i)) &
> +						GMAC_RXQCTRL_PSRQX_MASK(i));
>  
> -	value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> +	ctrl2 &= ~clear_mask;
> +	ctrl3 &= ~clear_mask;
> +
> +	if (queue < 4) {
> +		ctrl2 |= (prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
This is a bit of a nitpick but do you think it would make sense to replace that 
"4" with a macro? Something like GMAC_RXQCTRL_PSRXQ_MAXCTRL2QUEUE?

>  						GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	writel(value, ioaddr + base_register);
> +
> +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
I suppose that the order of these two writes are somehow important, else these 
could be factored out of the conditional block. Could you maybe add a short 
comment that explains why the order of these writes matter?

Best Regards,
Serge Semin Feb. 20, 2024, 10:08 a.m. UTC | #2 
On Mon, Feb 19, 2024 at 11:24:05AM +0100, Piotr Wejman wrote:
> The driver should ensure that same priority is not mapped to multiple
> rx queues. Currently dwmac4_rx_queue_priority function is adding
> priorities for a queue without clearing them from others.
> 
> From DesignWare Cores Ethernet Quality-of-Service
> Databook, section 17.1.29 MAC_RxQ_Ctrl2:
> "[...]The software must ensure that the content of this field is
> mutually exclusive to the PSRQ fields for other queues, that is,
> the same priority is not mapped to multiple Rx queues[...]"
> 
> After this patch, dwmac4_rx_queue_priority function will:
> - assign desired priorities to a queue
> - remove those priorities from all other queues
> The write sequence of CTRL2 and CTRL3 registers is done in the way to
> ensure this order.
> 

Thanks for the fix. The change in general seems good. The same is
applicable for the DW XGMAC too. Could you please apply it to
dwxgmac2_rx_queue_prio()?

> Also, the PSRQn field contains the mask of priorities and not only one
> priority. Rename "prio" argument to "prio_mask".

Please move this to a separate patch applied on top of the main change
described above. Also in order to be done coherently the renaming
should be extended onto all the Tx/Rx queue prio parts in the
driver:
0. dwmac4_core.c
   +-> dwmac4_rx_queue_priority()
   +-> dwmac4_tx_queue_priority()
1. dwxgmac2_core.c
   +-> dwxgmac2_rx_queue_prio()
   +-> dwxgmac2_tx_queue_prio()
2. hwif.h
   +-> stmmac_ops::rx_queue_prio
   +-> stmmac_ops::tx_queue_prio
3. stmmac.h
   +-> stmmac_rxq_cfg::prio
   +-> stmmac_txq_cfg::prio
4. stmmac_main.c:
   +-> stmmac_mac_config_rx_queues_prio()::prio
   +-> stmmac_mac_config_tx_queues_prio()::prio

* Hope I listed all of them.

-Serge(y)

> 
> Signed-off-by: Piotr Wejman <piotrwejman90@gmail.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 +++++++++++++------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> index 6b6d0de09619..6acc8bad794e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> @@ -89,22 +89,38 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw,
>  }
>  
>  static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
> -				     u32 prio, u32 queue)
> +				     u32 prio_mask, u32 queue)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
> -	u32 base_register;
> -	u32 value;
> +	u32 clear_mask = 0;
> +	u32 ctrl2, ctrl3;
> +	int i;
>  
> -	base_register = (queue < 4) ? GMAC_RXQ_CTRL2 : GMAC_RXQ_CTRL3;
> -	if (queue >= 4)
> -		queue -= 4;
> +	ctrl2 = readl(ioaddr + GMAC_RXQ_CTRL2);
> +	ctrl3 = readl(ioaddr + GMAC_RXQ_CTRL3);
>  
> -	value = readl(ioaddr + base_register);
> +	for (i = 0; i < 4; i++)
> +		clear_mask |= ((prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(i)) &
> +						GMAC_RXQCTRL_PSRQX_MASK(i));
>  
> -	value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> +	ctrl2 &= ~clear_mask;
> +	ctrl3 &= ~clear_mask;
> +
> +	if (queue < 4) {
> +		ctrl2 |= (prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
>  						GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	writel(value, ioaddr + base_register);
> +
> +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
> +	} else {
> +		queue -= 4;
> +
> +		ctrl3 |= (prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> +						GMAC_RXQCTRL_PSRQX_MASK(queue);
> +
> +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
> +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> +	}
>  }
>  
>  static void dwmac4_tx_queue_priority(struct mac_device_info *hw,
> -- 
> 2.25.1
> 
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index 6b6d0de09619..6acc8bad794e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -89,22 +89,38 @@  static void dwmac4_rx_queue_enable(struct mac_device_info *hw,
 }
 
 static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
-				     u32 prio, u32 queue)
+				     u32 prio_mask, u32 queue)
 {
 	void __iomem *ioaddr = hw->pcsr;
-	u32 base_register;
-	u32 value;
+	u32 clear_mask = 0;
+	u32 ctrl2, ctrl3;
+	int i;
 
-	base_register = (queue < 4) ? GMAC_RXQ_CTRL2 : GMAC_RXQ_CTRL3;
-	if (queue >= 4)
-		queue -= 4;
+	ctrl2 = readl(ioaddr + GMAC_RXQ_CTRL2);
+	ctrl3 = readl(ioaddr + GMAC_RXQ_CTRL3);
 
-	value = readl(ioaddr + base_register);
+	for (i = 0; i < 4; i++)
+		clear_mask |= ((prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(i)) &
+						GMAC_RXQCTRL_PSRQX_MASK(i));
 
-	value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
-	value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
+	ctrl2 &= ~clear_mask;
+	ctrl3 &= ~clear_mask;
+
+	if (queue < 4) {
+		ctrl2 |= (prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
 						GMAC_RXQCTRL_PSRQX_MASK(queue);
-	writel(value, ioaddr + base_register);
+
+		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
+		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
+	} else {
+		queue -= 4;
+
+		ctrl3 |= (prio_mask << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
+						GMAC_RXQCTRL_PSRQX_MASK(queue);
+
+		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
+		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
+	}
 }
 
 static void dwmac4_tx_queue_priority(struct mac_device_info *hw,