* [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 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 ` 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 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: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: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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.