Re: [PATCH v2] net: stmmac: fix rx queue priority assignment

[Simon Horman]

On Tue, Feb 27, 2024 at 05:00:12PM +0000, Simon Horman wrote:
> On Mon, Feb 26, 2024 at 10:31:44AM +0100, Piotr Wejman wrote:
> > The driver should ensure that same priority is not mapped to multiple
> > rx queues. Currently 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, 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.
> > 
> > Signed-off-by: Piotr Wejman <piotrwejman90@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Add some comments
> >   - Apply same changes to dwxgmac2_rx_queue_prio()
> >   - Revert "Rename prio argument to prio_mask"
> >   - Link to v1: https://lore.kernel.org/netdev/20240219102405.32015-1-piotrwejman90@xxxxxxxxx/T/#u
> > 
> >  .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 42 +++++++++++++++----
> >  .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 40 ++++++++++++++----
> >  2 files changed, 66 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > index 6b6d0de09619..76ec0c1da250 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > @@ -92,19 +92,43 @@ static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
> >  				     u32 prio, 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);
> > +	
> > +	/* The software must ensure that the same priority
> > +	 * is not mapped to multiple Rx queues.
> > +	 */
> > +	for (i = 0; i < 4; i++)
> > +		clear_mask |= ((prio << GMAC_RXQCTRL_PSRQX_SHIFT(i)) &
> > +						GMAC_RXQCTRL_PSRQX_MASK(i));
> >  
> > -	value = readl(ioaddr + base_register);
> > +	ctrl2 &= ~clear_mask;
> > +	ctrl3 &= ~clear_mask;
> >  
> > -	value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
> > -	value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> > +	/* Assign new priorities to a queue and
> > +	 * clear them from others queues.
> > +	 * The CTRL2 and CTRL3 registers write sequence is done
> > +	 * in the way to ensure this order.
> > +	 */
> > +	if (queue < 4) {
> > +		ctrl2 |= (prio << 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 << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> > +						GMAC_RXQCTRL_PSRQX_MASK(queue);
> > +
> > +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
> > +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> > +	}
> >  }
> 
> Hi Piotr,
> 
> Sorry if I am on the wrong track here, but this seems a little odd to me.
> 
> My reading is that each byte of GMAC_RXQ_CTRL2 and GMAC_RXQ_CTRL3
> hold the priority value - an integer in the range of 0-255 - for
> each of 8 queues.

Thinking about this some more, and checking the code some more, I realise I
am wrong here. I now see that the priority values are bit-fields not
integers. So I think what you have is fine.

Sorry about the noise.