| Message ID | 20240219102405.32015-1-piotrwejman90@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: fix rx queue priority assignment | expand |
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,
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 --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,