Hi Joao,

Please see my in-line comments.

Regards,
Séraphin
--
Seraphin BONNAFFE | Tel: +33244027061
STMicroelectronics | ADG | S/W Design

On 12/20/2016 01:55 PM, Joao Pinto wrote:
When the hardware is synthesized with multiple queues, all queues are
disabled for default. This patch adds the rx queues configuration.
This patch was successfully tested in a Synopsys QoS Reference design.

Signed-off-by: Joao Pinto <jpi...@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h      |  2 ++
 drivers/net/ethernet/stmicro/stmmac/dwmac4.h      |  4 ++++
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 11 +++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++++
 4 files changed, 38 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h 
b/drivers/net/ethernet/stmicro/stmmac/common.h
index b13a144..61bab50 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -454,6 +454,8 @@ struct stmmac_ops {
        void (*core_init)(struct mac_device_info *hw, int mtu);
        /* Enable and verify that the IPC module is supported */
        int (*rx_ipc)(struct mac_device_info *hw);
+       /* Enable RX Queues */
+       void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue);
        /* Dump MAC registers */
        void (*dump_regs)(struct mac_device_info *hw);
        /* Handle extra events on specific interrupts hw dependent */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h 
b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
index 3e8d4fe..fd013bd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
@@ -22,6 +22,7 @@
 #define GMAC_HASH_TAB_32_63            0x00000014
 #define GMAC_RX_FLOW_CTRL              0x00000090
 #define GMAC_QX_TX_FLOW_CTRL(x)                (0x70 + x * 4)
+#define GMAC_RXQ_CTRL0                 0x000000a0
 #define GMAC_INT_STATUS                        0x000000b0
 #define GMAC_INT_EN                    0x000000b4
 #define GMAC_PCS_BASE                  0x000000e0
@@ -44,6 +45,9 @@

 #define GMAC_MAX_PERFECT_ADDRESSES     128

+/* MAC RX Queue Enable*/
+#define GMAC_RX_QUEUE_ENABLE(queue)    BIT(queue * 2)

According to the GMAC_RXQ0 register description from the dwc_ether_qos_databok, 
I guess this will enable the queues in AV only.
What if we would like to enable them in DCB (10)b ?


+
 /* MAC Flow Control RX */
 #define GMAC_RX_FLOW_CTRL_RFE          BIT(0)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index eaed7cb..7ec1887 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -59,6 +59,16 @@ static void dwmac4_core_init(struct mac_device_info *hw, int 
mtu)
        writel(value, ioaddr + GMAC_INT_EN);
 }

+static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue)
+{
+       void __iomem *ioaddr = hw->pcsr;
+       u32 value = readl(ioaddr + GMAC_RXQ_CTRL0);
+
+       value |= GMAC_RX_QUEUE_ENABLE(queue);

What happen if for some reason the previous value of the register was 0xffff ?
The OR mask, by itself, won't clear the bits. AFAIU each queue is enabled with 
a 2-bit value, and (11)b don't have the same effect as (01)b, does it ?

I would suggest to clear the RXQ-enable bits before writing a new value.
Something like
  value &= GMAC_RX_QUEUE_CLEAR(queue)
  value |= GMAC_RX_QUEUE_ENABLE(queue);

What do you think about that ?

+
+       writel(value, ioaddr + GMAC_RXQ_CTRL0);
+}
+
 static void dwmac4_dump_regs(struct mac_device_info *hw)
 {
        void __iomem *ioaddr = hw->pcsr;
@@ -392,6 +402,7 @@ static void dwmac4_debug(void __iomem *ioaddr, struct 
stmmac_extra_stats *x)
 static const struct stmmac_ops dwmac4_ops = {
        .core_init = dwmac4_core_init,
        .rx_ipc = dwmac4_rx_ipc_enable,
+       .rx_queue_enable = dwmac4_rx_queue_enable,
        .dump_regs = dwmac4_dump_regs,
        .host_irq_status = dwmac4_irq_status,
        .flow_ctrl = dwmac4_flow_ctrl,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..e30034d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1271,6 +1271,24 @@ static void free_dma_desc_resources(struct stmmac_priv 
*priv)
 }

 /**
+ *  stmmac_mac_enable_rx_queues - Enable MAC rx queues
+ *  @priv: driver private structure
+ *  Description: It is used for enabling the rx queues in the MAC
+ */
+static void stmmac_mac_enable_rx_queues(struct stmmac_priv *priv)
+{
+       int rx_count = priv->dma_cap.number_rx_channel;
+       int queue = 0;
+
+       /* If GMAC does not have multiqueues, then this is not necessary*/
+       if (rx_count == 1)
+               return;
+
+       for (queue = 0; queue < rx_count; queue++)
+               priv->hw->mac->rx_queue_enable(priv->hw, queue);


Maybe it is worth checking if (priv->hw->mac->rx_queue_enable)
before actually calling this callback ?
I can see you have implemented it for dwmac4, but not for dwmac100 and dwmac100
In that case we may have a null pointer exception here.

+}
+
+/**
  *  stmmac_dma_operation_mode - HW DMA operation mode
  *  @priv: driver private structure
  *  Description: it is used for configuring the DMA operation mode register in
@@ -1691,6 +1709,9 @@ static int stmmac_hw_setup(struct net_device *dev, bool 
init_ptp)
        /* Initialize the MAC Core */
        priv->hw->mac->core_init(priv->hw, dev->mtu);

+       /* Initialize MAC RX Queues */
+       stmmac_mac_enable_rx_queues(priv);
+
        ret = priv->hw->mac->rx_ipc(priv->hw);
        if (!ret) {
                netdev_warn(priv->dev, "RX IPC Checksum Offload disabled\n");

Reply via email to