* [PATCH] stmmac: enable rx queues
@ 2016-12-20 12:55 Joao Pinto
2016-12-20 14:43 ` Niklas Cassel
2016-12-20 14:52 ` Seraphin BONNAFFE
0 siblings, 2 replies; 8+ messages in thread
From: Joao Pinto @ 2016-12-20 12:55 UTC (permalink / raw)
To: peppe.cavallaro, davem
Cc: hock.leong.kweh, niklas.cassel, pavel, linux-kernel, netdev, Joao Pinto
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 <jpinto@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)
+
/* 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);
+
+ 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);
+}
+
+/**
* 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");
--
2.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 12:55 [PATCH] stmmac: enable rx queues Joao Pinto
@ 2016-12-20 14:43 ` Niklas Cassel
2016-12-20 14:52 ` Joao Pinto
2016-12-20 14:52 ` Seraphin BONNAFFE
1 sibling, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2016-12-20 14:43 UTC (permalink / raw)
To: Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, pavel, linux-kernel, netdev
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 <jpinto@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)
Always have parentheses around a variable in a
macro, otherwise strange things could happen.
Imagine if you send 5 - 4 as argument,
it will then expand to 5 - 4 * 2 = -3,
instead of (5 - 4) * 2 = 2
> +
> /* 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);
> +
> + 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;
priv->dma_cap.number_rx_channel
actually contains the value from register
MAC_HW_Feature2, field RXCHCNT,
which is the number of DMA rx channels.
This is not the same as the number of MTL
receive queues, field RXQCNT in MAC_HW_Feature2.
I guess they will often have the same value,
but since there actually are two different fields
for them, I suppose that is not always the case.
Reading the comments in dwmac4_dma.*
#define DMA_CHANNEL_NB_MAX 1
"Only Channel 0 is actually configured and used"
"Following code only done for channel 0, other channels not yet supported"
Is there any point in actually enabling more than RX queue 0 if the
driver does not yet support more than one channel.
Can RXCHCNT ever be different from RXQCNT?
If so, when? Maybe when using an external DMA IP?
> + 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);
> +}
> +
> +/**
> * 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");
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 12:55 [PATCH] stmmac: enable rx queues Joao Pinto
2016-12-20 14:43 ` Niklas Cassel
@ 2016-12-20 14:52 ` Seraphin BONNAFFE
2016-12-20 14:59 ` Joao Pinto
1 sibling, 1 reply; 8+ messages in thread
From: Seraphin BONNAFFE @ 2016-12-20 14:52 UTC (permalink / raw)
To: Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, niklas.cassel, pavel, linux-kernel, netdev
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 <jpinto@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");
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 14:43 ` Niklas Cassel
@ 2016-12-20 14:52 ` Joao Pinto
2016-12-20 15:05 ` Niklas Cassel
0 siblings, 1 reply; 8+ messages in thread
From: Joao Pinto @ 2016-12-20 14:52 UTC (permalink / raw)
To: Niklas Cassel, Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, pavel, linux-kernel, netdev
Hi Niklas,
Às 2:43 PM de 12/20/2016, Niklas Cassel escreveu:
>
>
> 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 <jpinto@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)
>
> Always have parentheses around a variable in a
> macro, otherwise strange things could happen.
> Imagine if you send 5 - 4 as argument,
> it will then expand to 5 - 4 * 2 = -3,
> instead of (5 - 4) * 2 = 2
Right. I am going to do that.
>
>> +
>> /* 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);
>> +
>> + 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;
>
> priv->dma_cap.number_rx_channel
> actually contains the value from register
> MAC_HW_Feature2, field RXCHCNT,
> which is the number of DMA rx channels.
>
> This is not the same as the number of MTL
> receive queues, field RXQCNT in MAC_HW_Feature2.
>
> I guess they will often have the same value,
> but since there actually are two different fields
> for them, I suppose that is not always the case.
Yes, you typically have a match between channels and queues.
But I can use RXQCNT of course, I agree.
>
>
>
> Reading the comments in dwmac4_dma.*
>
> #define DMA_CHANNEL_NB_MAX 1
>
> "Only Channel 0 is actually configured and used"
>
> "Following code only done for channel 0, other channels not yet supported"
>
> Is there any point in actually enabling more than RX queue 0 if the
> driver does not yet support more than one channel.
> Can RXCHCNT ever be different from RXQCNT?
> If so, when? Maybe when using an external DMA IP?
Yes, currently stmmac only supports 1 Channel. Bt it needs this feature if the
hardware is multi-channel. The hardware I have is multi-channel and so you have
to enable RX queue for it to work and that's why I made this fix.
In the future I will develope multi channel support for stmmac and this RX queue
enable will be already made.
>
>
>> + 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);
>> +}
>> +
>> +/**
>> * 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");
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 14:52 ` Seraphin BONNAFFE
@ 2016-12-20 14:59 ` Joao Pinto
2016-12-20 15:31 ` Seraphin BONNAFFE
0 siblings, 1 reply; 8+ messages in thread
From: Joao Pinto @ 2016-12-20 14:59 UTC (permalink / raw)
To: Seraphin BONNAFFE, Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, niklas.cassel, pavel, linux-kernel, netdev
Hi Séraphin,
Às 2:52 PM de 12/20/2016, Seraphin BONNAFFE escreveu:
> 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 <jpinto@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 ?
Correct, I am enabling AV only. What you are saying makes perfect sense. What
about adding a variable to plat in order to configure the RX queue type?
Example:
plat->rx_queue_type = RX_QUEUE_DCB or plat->rx_queue_type = RX_QUEUE_AV
>
>
>> +
>> /* 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 ?
According to the databook, these values are always reset, but we can force it to
clear as you suggest.
>
>> +
>> + 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.
Yes, you are absolutely correct. Since I am using gmac4, I forgot that stmmac
can use other types.
>
>> +}
>> +
>> +/**
>> * 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");
>>
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 14:52 ` Joao Pinto
@ 2016-12-20 15:05 ` Niklas Cassel
2016-12-20 15:09 ` Joao Pinto
0 siblings, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2016-12-20 15:05 UTC (permalink / raw)
To: Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, pavel, linux-kernel, netdev
On 12/20/2016 03:52 PM, Joao Pinto wrote:
> Hi Niklas,
>
> Às 2:43 PM de 12/20/2016, Niklas Cassel escreveu:
>>
>> 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 <jpinto@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)
>> Always have parentheses around a variable in a
>> macro, otherwise strange things could happen.
>> Imagine if you send 5 - 4 as argument,
>> it will then expand to 5 - 4 * 2 = -3,
>> instead of (5 - 4) * 2 = 2
> Right. I am going to do that.
>
>>> +
>>> /* 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);
>>> +
>>> + 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;
>> priv->dma_cap.number_rx_channel
>> actually contains the value from register
>> MAC_HW_Feature2, field RXCHCNT,
>> which is the number of DMA rx channels.
>>
>> This is not the same as the number of MTL
>> receive queues, field RXQCNT in MAC_HW_Feature2.
>>
>> I guess they will often have the same value,
>> but since there actually are two different fields
>> for them, I suppose that is not always the case.
> Yes, you typically have a match between channels and queues.
> But I can use RXQCNT of course, I agree.
>
>>
>>
>> Reading the comments in dwmac4_dma.*
>>
>> #define DMA_CHANNEL_NB_MAX 1
>>
>> "Only Channel 0 is actually configured and used"
>>
>> "Following code only done for channel 0, other channels not yet supported"
>>
>> Is there any point in actually enabling more than RX queue 0 if the
>> driver does not yet support more than one channel.
>> Can RXCHCNT ever be different from RXQCNT?
>> If so, when? Maybe when using an external DMA IP?
> Yes, currently stmmac only supports 1 Channel. Bt it needs this feature if the
> hardware is multi-channel. The hardware I have is multi-channel and so you have
> to enable RX queue for it to work and that's why I made this fix.
> In the future I will develope multi channel support for stmmac and this RX queue
> enable will be already made.
I understand that for multi-queue hardware, RX queue 0 is default off,
but perhaps it is safer to only enable RX queue 0,
even if you have more than one RX queue.
(Only until you have implemented actual support for multi-queues
in the driver.)
But if you know that it's safe to enable all RX queues even if the
driver only uses RX queue 0, then perhaps it doesn't matter.
>
>>
>>> + 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);
>>> +}
>>> +
>>> +/**
>>> * 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");
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 15:05 ` Niklas Cassel
@ 2016-12-20 15:09 ` Joao Pinto
0 siblings, 0 replies; 8+ messages in thread
From: Joao Pinto @ 2016-12-20 15:09 UTC (permalink / raw)
To: Niklas Cassel, Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, pavel, linux-kernel, netdev
Às 3:05 PM de 12/20/2016, Niklas Cassel escreveu:
>
>
> On 12/20/2016 03:52 PM, Joao Pinto wrote:
>> Hi Niklas,
>>
>> Às 2:43 PM de 12/20/2016, Niklas Cassel escreveu:
>>>
>>> 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 <jpinto@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)
>>> Always have parentheses around a variable in a
>>> macro, otherwise strange things could happen.
>>> Imagine if you send 5 - 4 as argument,
>>> it will then expand to 5 - 4 * 2 = -3,
>>> instead of (5 - 4) * 2 = 2
>> Right. I am going to do that.
>>
>>>> +
>>>> /* 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);
>>>> +
>>>> + 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;
>>> priv->dma_cap.number_rx_channel
>>> actually contains the value from register
>>> MAC_HW_Feature2, field RXCHCNT,
>>> which is the number of DMA rx channels.
>>>
>>> This is not the same as the number of MTL
>>> receive queues, field RXQCNT in MAC_HW_Feature2.
>>>
>>> I guess they will often have the same value,
>>> but since there actually are two different fields
>>> for them, I suppose that is not always the case.
>> Yes, you typically have a match between channels and queues.
>> But I can use RXQCNT of course, I agree.
>>
>>>
>>>
>>> Reading the comments in dwmac4_dma.*
>>>
>>> #define DMA_CHANNEL_NB_MAX 1
>>>
>>> "Only Channel 0 is actually configured and used"
>>>
>>> "Following code only done for channel 0, other channels not yet supported"
>>>
>>> Is there any point in actually enabling more than RX queue 0 if the
>>> driver does not yet support more than one channel.
>>> Can RXCHCNT ever be different from RXQCNT?
>>> If so, when? Maybe when using an external DMA IP?
>> Yes, currently stmmac only supports 1 Channel. Bt it needs this feature if the
>> hardware is multi-channel. The hardware I have is multi-channel and so you have
>> to enable RX queue for it to work and that's why I made this fix.
>> In the future I will develope multi channel support for stmmac and this RX queue
>> enable will be already made.
>
> I understand that for multi-queue hardware, RX queue 0 is default off,
> but perhaps it is safer to only enable RX queue 0,
> even if you have more than one RX queue.
> (Only until you have implemented actual support for multi-queues
> in the driver.)
>
> But if you know that it's safe to enable all RX queues even if the
> driver only uses RX queue 0, then perhaps it doesn't matter.
I think it won't bring problems to enable all the available queues even if the
driver only uses queue 0. My QoS reference design has 4 RX queues and it works fine.
>
>
>>
>>>
>>>> + 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);
>>>> +}
>>>> +
>>>> +/**
>>>> * 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");
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: enable rx queues
2016-12-20 14:59 ` Joao Pinto
@ 2016-12-20 15:31 ` Seraphin BONNAFFE
0 siblings, 0 replies; 8+ messages in thread
From: Seraphin BONNAFFE @ 2016-12-20 15:31 UTC (permalink / raw)
To: Joao Pinto, peppe.cavallaro, davem
Cc: hock.leong.kweh, niklas.cassel, pavel, linux-kernel, netdev
Hi Joao,
On 12/20/2016 03:59 PM, Joao Pinto wrote:
>
> Hi Séraphin,
>
> Às 2:52 PM de 12/20/2016, Seraphin BONNAFFE escreveu:
>> 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 <jpinto@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 ?
>
> Correct, I am enabling AV only. What you are saying makes perfect sense. What
> about adding a variable to plat in order to configure the RX queue type?
> Example:
> plat->rx_queue_type = RX_QUEUE_DCB or plat->rx_queue_type = RX_QUEUE_AV
>
I am just not 100% sure about what does enabling the Queues in DCB implies,
and I think it needs something more to support the DCB feature as a whole in the driver, which is not supported yet.
On the other side, I observed that the queues should be enabled in AV by default.
Maybe providing this DCB option to plat could bring some confusion.
I would rather specify in the Macro name or in the comment above, that we are enabling the Queue in AV,
and eventually provide the same macro for enabling queues in DCB.
>
>>
>>
>>> +
>>> /* 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 ?
>
> According to the databook, these values are always reset, but we can force it to
> clear as you suggest.
>
Yes, just in case. We may have another driver controlling the IP in a bootloader for example, and we don't know in which state it will let the controller.
...unless we have a IP reset at stmmac init (?). Anyway, it is more careful to control what we write to that register.
>>
>>> +
>>> + 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.
>
> Yes, you are absolutely correct. Since I am using gmac4, I forgot that stmmac
> can use other types.
>
By the way, do you have any idea if this RxQ enabling is necessary for dwmac version before 4.00?
Regards,
Séraphin
>>
>>> +}
>>> +
>>> +/**
>>> * 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");
>>>
>
> Thanks.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-20 15:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-20 12:55 [PATCH] stmmac: enable rx queues Joao Pinto
2016-12-20 14:43 ` Niklas Cassel
2016-12-20 14:52 ` Joao Pinto
2016-12-20 15:05 ` Niklas Cassel
2016-12-20 15:09 ` Joao Pinto
2016-12-20 14:52 ` Seraphin BONNAFFE
2016-12-20 14:59 ` Joao Pinto
2016-12-20 15:31 ` Seraphin BONNAFFE
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).