| Message ID | aa027812f1ab084c3d1e578369cd35c3be3b7f96.1489169213.git.jpinto@synopsys.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > This patch adds the RX and TX scheduling algorithms programming. > It introduces the multiple queues configuration function > (stmmac_mtl_configuration) in stmmac_main. > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> > --- > Changes v4->v5: > - patch title update (stmicro replaced by stmmac) > Changes v3->v4: > - Just to keep up with patch-set version > Changes v2->v3: > - Switch statements with a tab > Changes v1->v2: > - Just to keep up with patch-set version > > drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > 4 files changed, 90 insertions(+), 3 deletions(-) This patch breaks backwards-compatibility with DTBs that don't have an of the multiple queue properties. See below... > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index 04d9245..5a0a781 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -455,6 +455,10 @@ struct stmmac_ops { > int (*rx_ipc)(struct mac_device_info *hw); > /* Enable RX Queues */ > void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > + /* Program RX Algorithms */ > + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > + /* Program TX Algorithms */ > + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > /* Dump MAC registers */ > void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > /* 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 db45134..748ab6f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > @@ -161,6 +161,16 @@ enum power_event { > #define GMAC_HI_REG_AE BIT(31) > > /* MTL registers */ > +#define MTL_OPERATION_MODE 0x00000c00 > +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > +#define MTL_OPERATION_RAA BIT(2) > +#define MTL_OPERATION_RAA_SP (0x0 << 2) > +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > + > #define MTL_INT_STATUS 0x00000c20 > #define MTL_INT_Q0 BIT(0) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > index 1e79e65..f966755 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > writel(value, ioaddr + GMAC_RXQ_CTRL0); > } > > +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > + u32 rx_alg) > +{ > + void __iomem *ioaddr = hw->pcsr; > + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > + > + value &= ~MTL_OPERATION_RAA; > + switch (rx_alg) { > + case MTL_RX_ALGORITHM_SP: > + value |= MTL_OPERATION_RAA_SP; > + break; > + case MTL_RX_ALGORITHM_WSP: > + value |= MTL_OPERATION_RAA_WSP; > + break; > + default: > + break; > + } > + > + writel(value, ioaddr + MTL_OPERATION_MODE); > +} > + > +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > + u32 tx_alg) > +{ > + void __iomem *ioaddr = hw->pcsr; > + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > + > + value &= ~MTL_OPERATION_SCHALG_MASK; > + switch (tx_alg) { > + case MTL_TX_ALGORITHM_WRR: > + value |= MTL_OPERATION_SCHALG_WRR; > + break; > + case MTL_TX_ALGORITHM_WFQ: > + value |= MTL_OPERATION_SCHALG_WFQ; > + break; > + case MTL_TX_ALGORITHM_DWRR: > + value |= MTL_OPERATION_SCHALG_DWRR; > + break; > + case MTL_TX_ALGORITHM_SP: > + value |= MTL_OPERATION_SCHALG_SP; > + break; > + default: > + break; > + } > +} > + > static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > { > void __iomem *ioaddr = hw->pcsr; > @@ -457,6 +503,8 @@ 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, > + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > .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 4498a38..af57f8d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > } > > /** > + * stmmac_mtl_configuration - Configure MTL > + * @priv: driver private structure > + * Description: It is used for configurring MTL > + */ > +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > +{ > + u32 rx_queues_count = priv->plat->rx_queues_to_use; > + u32 tx_queues_count = priv->plat->tx_queues_to_use; > + > + /* Configure MTL RX algorithms */ > + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > + priv->plat->rx_sched_algorithm); > + > + /* Configure MTL TX algorithms */ > + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > + priv->plat->tx_sched_algorithm); > + > + /* Enable MAC RX Queues */ > + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > + stmmac_mac_enable_rx_queues(priv); This is almost equivalent to the code removed from stmmac_hw_setup() which happens to be the key for this driver to work for me. However, the code above adds an additional check for rx_queues_count > 1 which is going to be false for any existing DTB, because it is derived from the values retrieved from new device tree properties. So I think for backwards compatibility we'd need something like this: if ((rx_queue_count == 0 || rx_queue_count > 1) && priv->hw->mac->rx_queue_enable) But then I'm beginning to think maybe we don't need a check here at all because it would only prevent RX queue setup for rx_queue_count == 1 and I think it would still be legitimate to set it up even then. stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that is derived from the number of RX queues derived from the feature registers and therefore refers to the number of queues that the hardware supports as opposed to the number of queues configured in device tree. I can follow up with a patch to restore backwards-compatibility. Thierry
Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >> This patch adds the RX and TX scheduling algorithms programming. >> It introduces the multiple queues configuration function >> (stmmac_mtl_configuration) in stmmac_main. >> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >> --- >> Changes v4->v5: >> - patch title update (stmicro replaced by stmmac) >> Changes v3->v4: >> - Just to keep up with patch-set version >> Changes v2->v3: >> - Switch statements with a tab >> Changes v1->v2: >> - Just to keep up with patch-set version >> >> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >> 4 files changed, 90 insertions(+), 3 deletions(-) > > This patch breaks backwards-compatibility with DTBs that don't have an > of the multiple queue properties. > > See below... > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >> index 04d9245..5a0a781 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >> @@ -455,6 +455,10 @@ struct stmmac_ops { >> int (*rx_ipc)(struct mac_device_info *hw); >> /* Enable RX Queues */ >> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >> + /* Program RX Algorithms */ >> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >> + /* Program TX Algorithms */ >> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >> /* Dump MAC registers */ >> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >> /* 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 db45134..748ab6f 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >> @@ -161,6 +161,16 @@ enum power_event { >> #define GMAC_HI_REG_AE BIT(31) >> >> /* MTL registers */ >> +#define MTL_OPERATION_MODE 0x00000c00 >> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >> +#define MTL_OPERATION_RAA BIT(2) >> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >> + >> #define MTL_INT_STATUS 0x00000c20 >> #define MTL_INT_Q0 BIT(0) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >> index 1e79e65..f966755 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >> writel(value, ioaddr + GMAC_RXQ_CTRL0); >> } >> >> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >> + u32 rx_alg) >> +{ >> + void __iomem *ioaddr = hw->pcsr; >> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >> + >> + value &= ~MTL_OPERATION_RAA; >> + switch (rx_alg) { >> + case MTL_RX_ALGORITHM_SP: >> + value |= MTL_OPERATION_RAA_SP; >> + break; >> + case MTL_RX_ALGORITHM_WSP: >> + value |= MTL_OPERATION_RAA_WSP; >> + break; >> + default: >> + break; >> + } >> + >> + writel(value, ioaddr + MTL_OPERATION_MODE); >> +} >> + >> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >> + u32 tx_alg) >> +{ >> + void __iomem *ioaddr = hw->pcsr; >> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >> + >> + value &= ~MTL_OPERATION_SCHALG_MASK; >> + switch (tx_alg) { >> + case MTL_TX_ALGORITHM_WRR: >> + value |= MTL_OPERATION_SCHALG_WRR; >> + break; >> + case MTL_TX_ALGORITHM_WFQ: >> + value |= MTL_OPERATION_SCHALG_WFQ; >> + break; >> + case MTL_TX_ALGORITHM_DWRR: >> + value |= MTL_OPERATION_SCHALG_DWRR; >> + break; >> + case MTL_TX_ALGORITHM_SP: >> + value |= MTL_OPERATION_SCHALG_SP; >> + break; >> + default: >> + break; >> + } >> +} >> + >> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >> { >> void __iomem *ioaddr = hw->pcsr; >> @@ -457,6 +503,8 @@ 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, >> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >> .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 4498a38..af57f8d 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >> } >> >> /** >> + * stmmac_mtl_configuration - Configure MTL >> + * @priv: driver private structure >> + * Description: It is used for configurring MTL >> + */ >> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >> +{ >> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >> + >> + /* Configure MTL RX algorithms */ >> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >> + priv->plat->rx_sched_algorithm); >> + >> + /* Configure MTL TX algorithms */ >> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >> + priv->plat->tx_sched_algorithm); >> + >> + /* Enable MAC RX Queues */ >> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >> + stmmac_mac_enable_rx_queues(priv); > > This is almost equivalent to the code removed from stmmac_hw_setup() > which happens to be the key for this driver to work for me. However, the > code above adds an additional check for rx_queues_count > 1 which is > going to be false for any existing DTB, because it is derived from the > values retrieved from new device tree properties. > > So I think for backwards compatibility we'd need something like this: > > if ((rx_queue_count == 0 || rx_queue_count > 1) && > priv->hw->mac->rx_queue_enable) > > But then I'm beginning to think maybe we don't need a check here at all > because it would only prevent RX queue setup for rx_queue_count == 1 and > I think it would still be legitimate to set it up even then. > > stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > is derived from the number of RX queues derived from the feature > registers and therefore refers to the number of queues that the hardware > supports as opposed to the number of queues configured in device tree. > > I can follow up with a patch to restore backwards-compatibility. Forhw configured as single queue you don't need to enable the rx queue, since they are enable by default. if you check in stmmac_platform.c I assure backward compatibility by setting the number of rx and tx queues = 1 if nothing is declared. Please check here: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#n156 Thanks for reviewing. > > Thierry >
On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: > Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > > On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > >> This patch adds the RX and TX scheduling algorithms programming. > >> It introduces the multiple queues configuration function > >> (stmmac_mtl_configuration) in stmmac_main. > >> > >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >> --- > >> Changes v4->v5: > >> - patch title update (stmicro replaced by stmmac) > >> Changes v3->v4: > >> - Just to keep up with patch-set version > >> Changes v2->v3: > >> - Switch statements with a tab > >> Changes v1->v2: > >> - Just to keep up with patch-set version > >> > >> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > >> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > >> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > >> 4 files changed, 90 insertions(+), 3 deletions(-) > > > > This patch breaks backwards-compatibility with DTBs that don't have an > > of the multiple queue properties. > > > > See below... > > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >> index 04d9245..5a0a781 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >> @@ -455,6 +455,10 @@ struct stmmac_ops { > >> int (*rx_ipc)(struct mac_device_info *hw); > >> /* Enable RX Queues */ > >> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > >> + /* Program RX Algorithms */ > >> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > >> + /* Program TX Algorithms */ > >> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > >> /* Dump MAC registers */ > >> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > >> /* 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 db45134..748ab6f 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >> @@ -161,6 +161,16 @@ enum power_event { > >> #define GMAC_HI_REG_AE BIT(31) > >> > >> /* MTL registers */ > >> +#define MTL_OPERATION_MODE 0x00000c00 > >> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > >> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > >> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > >> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > >> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > >> +#define MTL_OPERATION_RAA BIT(2) > >> +#define MTL_OPERATION_RAA_SP (0x0 << 2) > >> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > >> + > >> #define MTL_INT_STATUS 0x00000c20 > >> #define MTL_INT_Q0 BIT(0) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >> index 1e79e65..f966755 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > >> writel(value, ioaddr + GMAC_RXQ_CTRL0); > >> } > >> > >> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > >> + u32 rx_alg) > >> +{ > >> + void __iomem *ioaddr = hw->pcsr; > >> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >> + > >> + value &= ~MTL_OPERATION_RAA; > >> + switch (rx_alg) { > >> + case MTL_RX_ALGORITHM_SP: > >> + value |= MTL_OPERATION_RAA_SP; > >> + break; > >> + case MTL_RX_ALGORITHM_WSP: > >> + value |= MTL_OPERATION_RAA_WSP; > >> + break; > >> + default: > >> + break; > >> + } > >> + > >> + writel(value, ioaddr + MTL_OPERATION_MODE); > >> +} > >> + > >> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > >> + u32 tx_alg) > >> +{ > >> + void __iomem *ioaddr = hw->pcsr; > >> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >> + > >> + value &= ~MTL_OPERATION_SCHALG_MASK; > >> + switch (tx_alg) { > >> + case MTL_TX_ALGORITHM_WRR: > >> + value |= MTL_OPERATION_SCHALG_WRR; > >> + break; > >> + case MTL_TX_ALGORITHM_WFQ: > >> + value |= MTL_OPERATION_SCHALG_WFQ; > >> + break; > >> + case MTL_TX_ALGORITHM_DWRR: > >> + value |= MTL_OPERATION_SCHALG_DWRR; > >> + break; > >> + case MTL_TX_ALGORITHM_SP: > >> + value |= MTL_OPERATION_SCHALG_SP; > >> + break; > >> + default: > >> + break; > >> + } > >> +} > >> + > >> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > >> { > >> void __iomem *ioaddr = hw->pcsr; > >> @@ -457,6 +503,8 @@ 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, > >> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > >> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > >> .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 4498a38..af57f8d 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > >> } > >> > >> /** > >> + * stmmac_mtl_configuration - Configure MTL > >> + * @priv: driver private structure > >> + * Description: It is used for configurring MTL > >> + */ > >> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > >> +{ > >> + u32 rx_queues_count = priv->plat->rx_queues_to_use; > >> + u32 tx_queues_count = priv->plat->tx_queues_to_use; > >> + > >> + /* Configure MTL RX algorithms */ > >> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > >> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > >> + priv->plat->rx_sched_algorithm); > >> + > >> + /* Configure MTL TX algorithms */ > >> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > >> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > >> + priv->plat->tx_sched_algorithm); > >> + > >> + /* Enable MAC RX Queues */ > >> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > >> + stmmac_mac_enable_rx_queues(priv); > > > > This is almost equivalent to the code removed from stmmac_hw_setup() > > which happens to be the key for this driver to work for me. However, the > > code above adds an additional check for rx_queues_count > 1 which is > > going to be false for any existing DTB, because it is derived from the > > values retrieved from new device tree properties. > > > > So I think for backwards compatibility we'd need something like this: > > > > if ((rx_queue_count == 0 || rx_queue_count > 1) && > > priv->hw->mac->rx_queue_enable) > > > > But then I'm beginning to think maybe we don't need a check here at all > > because it would only prevent RX queue setup for rx_queue_count == 1 and > > I think it would still be legitimate to set it up even then. > > > > stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > > is derived from the number of RX queues derived from the feature > > registers and therefore refers to the number of queues that the hardware > > supports as opposed to the number of queues configured in device tree. > > > > I can follow up with a patch to restore backwards-compatibility. > > Forhw configured as single queue you don't need to enable the rx queue, since > they are enable by default. if you check in stmmac_platform.c I assure backward > compatibility by setting the number of rx and tx queues = 1 if nothing is > declared. Please check here: > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#n156 Ah yes, I just ran across that trying to debug why a subsequent patch broke things again. I think the rx_queue_count > 1 condition is still wrong in the above because it will still fail for the backwards- compatibility case. I still think any check other than priv->hw->mac->rx_queue_enable should be dropped when calling stmmac_mac_enable_rx_queues(). A later patch is going to move the check into that function anyway (via the for loop). I need to step through the series some more since there are a couple of other things I noticed that make the driver fail for Tegra186 now. Let me report back once I'm through all of it. Thierry
Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: > On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: >> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: >>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >>>> This patch adds the RX and TX scheduling algorithms programming. >>>> It introduces the multiple queues configuration function >>>> (stmmac_mtl_configuration) in stmmac_main. >>>> >>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>> --- >>>> Changes v4->v5: >>>> - patch title update (stmicro replaced by stmmac) >>>> Changes v3->v4: >>>> - Just to keep up with patch-set version >>>> Changes v2->v3: >>>> - Switch statements with a tab >>>> Changes v1->v2: >>>> - Just to keep up with patch-set version >>>> >>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >>>> 4 files changed, 90 insertions(+), 3 deletions(-) >>> >>> This patch breaks backwards-compatibility with DTBs that don't have an >>> of the multiple queue properties. >>> >>> See below... >>> >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>> index 04d9245..5a0a781 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>> @@ -455,6 +455,10 @@ struct stmmac_ops { >>>> int (*rx_ipc)(struct mac_device_info *hw); >>>> /* Enable RX Queues */ >>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >>>> + /* Program RX Algorithms */ >>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >>>> + /* Program TX Algorithms */ >>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >>>> /* Dump MAC registers */ >>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >>>> /* 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 db45134..748ab6f 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>> @@ -161,6 +161,16 @@ enum power_event { >>>> #define GMAC_HI_REG_AE BIT(31) >>>> >>>> /* MTL registers */ >>>> +#define MTL_OPERATION_MODE 0x00000c00 >>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >>>> +#define MTL_OPERATION_RAA BIT(2) >>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >>>> + >>>> #define MTL_INT_STATUS 0x00000c20 >>>> #define MTL_INT_Q0 BIT(0) >>>> >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>> index 1e79e65..f966755 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); >>>> } >>>> >>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >>>> + u32 rx_alg) >>>> +{ >>>> + void __iomem *ioaddr = hw->pcsr; >>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>> + >>>> + value &= ~MTL_OPERATION_RAA; >>>> + switch (rx_alg) { >>>> + case MTL_RX_ALGORITHM_SP: >>>> + value |= MTL_OPERATION_RAA_SP; >>>> + break; >>>> + case MTL_RX_ALGORITHM_WSP: >>>> + value |= MTL_OPERATION_RAA_WSP; >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> + >>>> + writel(value, ioaddr + MTL_OPERATION_MODE); >>>> +} >>>> + >>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >>>> + u32 tx_alg) >>>> +{ >>>> + void __iomem *ioaddr = hw->pcsr; >>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>> + >>>> + value &= ~MTL_OPERATION_SCHALG_MASK; >>>> + switch (tx_alg) { >>>> + case MTL_TX_ALGORITHM_WRR: >>>> + value |= MTL_OPERATION_SCHALG_WRR; >>>> + break; >>>> + case MTL_TX_ALGORITHM_WFQ: >>>> + value |= MTL_OPERATION_SCHALG_WFQ; >>>> + break; >>>> + case MTL_TX_ALGORITHM_DWRR: >>>> + value |= MTL_OPERATION_SCHALG_DWRR; >>>> + break; >>>> + case MTL_TX_ALGORITHM_SP: >>>> + value |= MTL_OPERATION_SCHALG_SP; >>>> + break; >>>> + default: >>>> + break; >>>> + } >>>> +} >>>> + >>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >>>> { >>>> void __iomem *ioaddr = hw->pcsr; >>>> @@ -457,6 +503,8 @@ 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, >>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >>>> .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 4498a38..af57f8d 100644 >>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >>>> } >>>> >>>> /** >>>> + * stmmac_mtl_configuration - Configure MTL >>>> + * @priv: driver private structure >>>> + * Description: It is used for configurring MTL >>>> + */ >>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >>>> +{ >>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >>>> + >>>> + /* Configure MTL RX algorithms */ >>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >>>> + priv->plat->rx_sched_algorithm); >>>> + >>>> + /* Configure MTL TX algorithms */ >>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >>>> + priv->plat->tx_sched_algorithm); >>>> + >>>> + /* Enable MAC RX Queues */ >>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >>>> + stmmac_mac_enable_rx_queues(priv); >>> >>> This is almost equivalent to the code removed from stmmac_hw_setup() >>> which happens to be the key for this driver to work for me. However, the >>> code above adds an additional check for rx_queues_count > 1 which is >>> going to be false for any existing DTB, because it is derived from the >>> values retrieved from new device tree properties. >>> >>> So I think for backwards compatibility we'd need something like this: >>> >>> if ((rx_queue_count == 0 || rx_queue_count > 1) && >>> priv->hw->mac->rx_queue_enable) >>> >>> But then I'm beginning to think maybe we don't need a check here at all >>> because it would only prevent RX queue setup for rx_queue_count == 1 and >>> I think it would still be legitimate to set it up even then. >>> >>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that >>> is derived from the number of RX queues derived from the feature >>> registers and therefore refers to the number of queues that the hardware >>> supports as opposed to the number of queues configured in device tree. >>> >>> I can follow up with a patch to restore backwards-compatibility. >> >> Forhw configured as single queue you don't need to enable the rx queue, since >> they are enable by default. if you check in stmmac_platform.c I assure backward >> compatibility by setting the number of rx and tx queues = 1 if nothing is >> declared. Please check here: >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= > > Ah yes, I just ran across that trying to debug why a subsequent patch > broke things again. I think the rx_queue_count > 1 condition is still > wrong in the above because it will still fail for the backwards- > compatibility case. Enabling RX Queue is needed only if the hw is configured as having more than 1 RX queue. If your hardware is configured as being multiple-queue, rx_queue_to_use should be > 1. True single queue hardware will work fine. > > I still think any check other than priv->hw->mac->rx_queue_enable should > be dropped when calling stmmac_mac_enable_rx_queues(). A later patch is > going to move the check into that function anyway (via the for loop). I > need to step through the series some more since there are a couple of > other things I noticed that make the driver fail for Tegra186 now. Let > me report back once I'm through all of it. > > Thierry >
On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: > Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: > > On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: > >> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > >>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > >>>> This patch adds the RX and TX scheduling algorithms programming. > >>>> It introduces the multiple queues configuration function > >>>> (stmmac_mtl_configuration) in stmmac_main. > >>>> > >>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >>>> --- > >>>> Changes v4->v5: > >>>> - patch title update (stmicro replaced by stmmac) > >>>> Changes v3->v4: > >>>> - Just to keep up with patch-set version > >>>> Changes v2->v3: > >>>> - Switch statements with a tab > >>>> Changes v1->v2: > >>>> - Just to keep up with patch-set version > >>>> > >>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > >>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > >>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > >>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > >>>> 4 files changed, 90 insertions(+), 3 deletions(-) > >>> > >>> This patch breaks backwards-compatibility with DTBs that don't have an > >>> of the multiple queue properties. > >>> > >>> See below... > >>> > >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>> index 04d9245..5a0a781 100644 > >>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>> @@ -455,6 +455,10 @@ struct stmmac_ops { > >>>> int (*rx_ipc)(struct mac_device_info *hw); > >>>> /* Enable RX Queues */ > >>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > >>>> + /* Program RX Algorithms */ > >>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > >>>> + /* Program TX Algorithms */ > >>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > >>>> /* Dump MAC registers */ > >>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > >>>> /* 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 db45134..748ab6f 100644 > >>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>> @@ -161,6 +161,16 @@ enum power_event { > >>>> #define GMAC_HI_REG_AE BIT(31) > >>>> > >>>> /* MTL registers */ > >>>> +#define MTL_OPERATION_MODE 0x00000c00 > >>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > >>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > >>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > >>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > >>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > >>>> +#define MTL_OPERATION_RAA BIT(2) > >>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) > >>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > >>>> + > >>>> #define MTL_INT_STATUS 0x00000c20 > >>>> #define MTL_INT_Q0 BIT(0) > >>>> > >>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>> index 1e79e65..f966755 100644 > >>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > >>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); > >>>> } > >>>> > >>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > >>>> + u32 rx_alg) > >>>> +{ > >>>> + void __iomem *ioaddr = hw->pcsr; > >>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>> + > >>>> + value &= ~MTL_OPERATION_RAA; > >>>> + switch (rx_alg) { > >>>> + case MTL_RX_ALGORITHM_SP: > >>>> + value |= MTL_OPERATION_RAA_SP; > >>>> + break; > >>>> + case MTL_RX_ALGORITHM_WSP: > >>>> + value |= MTL_OPERATION_RAA_WSP; > >>>> + break; > >>>> + default: > >>>> + break; > >>>> + } > >>>> + > >>>> + writel(value, ioaddr + MTL_OPERATION_MODE); > >>>> +} > >>>> + > >>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > >>>> + u32 tx_alg) > >>>> +{ > >>>> + void __iomem *ioaddr = hw->pcsr; > >>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>> + > >>>> + value &= ~MTL_OPERATION_SCHALG_MASK; > >>>> + switch (tx_alg) { > >>>> + case MTL_TX_ALGORITHM_WRR: > >>>> + value |= MTL_OPERATION_SCHALG_WRR; > >>>> + break; > >>>> + case MTL_TX_ALGORITHM_WFQ: > >>>> + value |= MTL_OPERATION_SCHALG_WFQ; > >>>> + break; > >>>> + case MTL_TX_ALGORITHM_DWRR: > >>>> + value |= MTL_OPERATION_SCHALG_DWRR; > >>>> + break; > >>>> + case MTL_TX_ALGORITHM_SP: > >>>> + value |= MTL_OPERATION_SCHALG_SP; > >>>> + break; > >>>> + default: > >>>> + break; > >>>> + } > >>>> +} > >>>> + > >>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > >>>> { > >>>> void __iomem *ioaddr = hw->pcsr; > >>>> @@ -457,6 +503,8 @@ 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, > >>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > >>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > >>>> .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 4498a38..af57f8d 100644 > >>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > >>>> } > >>>> > >>>> /** > >>>> + * stmmac_mtl_configuration - Configure MTL > >>>> + * @priv: driver private structure > >>>> + * Description: It is used for configurring MTL > >>>> + */ > >>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > >>>> +{ > >>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; > >>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; > >>>> + > >>>> + /* Configure MTL RX algorithms */ > >>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > >>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > >>>> + priv->plat->rx_sched_algorithm); > >>>> + > >>>> + /* Configure MTL TX algorithms */ > >>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > >>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > >>>> + priv->plat->tx_sched_algorithm); > >>>> + > >>>> + /* Enable MAC RX Queues */ > >>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > >>>> + stmmac_mac_enable_rx_queues(priv); > >>> > >>> This is almost equivalent to the code removed from stmmac_hw_setup() > >>> which happens to be the key for this driver to work for me. However, the > >>> code above adds an additional check for rx_queues_count > 1 which is > >>> going to be false for any existing DTB, because it is derived from the > >>> values retrieved from new device tree properties. > >>> > >>> So I think for backwards compatibility we'd need something like this: > >>> > >>> if ((rx_queue_count == 0 || rx_queue_count > 1) && > >>> priv->hw->mac->rx_queue_enable) > >>> > >>> But then I'm beginning to think maybe we don't need a check here at all > >>> because it would only prevent RX queue setup for rx_queue_count == 1 and > >>> I think it would still be legitimate to set it up even then. > >>> > >>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > >>> is derived from the number of RX queues derived from the feature > >>> registers and therefore refers to the number of queues that the hardware > >>> supports as opposed to the number of queues configured in device tree. > >>> > >>> I can follow up with a patch to restore backwards-compatibility. > >> > >> Forhw configured as single queue you don't need to enable the rx queue, since > >> they are enable by default. if you check in stmmac_platform.c I assure backward > >> compatibility by setting the number of rx and tx queues = 1 if nothing is > >> declared. Please check here: > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= > > > > Ah yes, I just ran across that trying to debug why a subsequent patch > > broke things again. I think the rx_queue_count > 1 condition is still > > wrong in the above because it will still fail for the backwards- > > compatibility case. > > Enabling RX Queue is needed only if the hw is configured as having more than 1 > RX queue. If your hardware is configured as being multiple-queue, > rx_queue_to_use should be > 1. True single queue hardware will work fine. But that's certainly not the case right now. The rx_queues_to_use field is initialized based on the new snps,rx-queues-to-use device tree property, or initialized to 1 in the absence of that property (and for PCI). Effectively that means all users of this driver will have their rx_queues_to_use field set to 1 until the DTS files are updated to set some other value. But that's not a requirement that we can impose. We still need to be able to deal with old device trees without regression. Even without the backwards-compatibility argument, wouldn't it make sense to allow users to restrict usage to a single RX queue even on hardware that supports multiple ones? It certainly works fine in my case, I don't see any advantage in forbidding it. Thierry
++Adding Corentin Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: > On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: >> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: >>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: >>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: >>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >>>>>> This patch adds the RX and TX scheduling algorithms programming. >>>>>> It introduces the multiple queues configuration function >>>>>> (stmmac_mtl_configuration) in stmmac_main. >>>>>> >>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>>>> --- >>>>>> Changes v4->v5: >>>>>> - patch title update (stmicro replaced by stmmac) >>>>>> Changes v3->v4: >>>>>> - Just to keep up with patch-set version >>>>>> Changes v2->v3: >>>>>> - Switch statements with a tab >>>>>> Changes v1->v2: >>>>>> - Just to keep up with patch-set version >>>>>> >>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) >>>>> >>>>> This patch breaks backwards-compatibility with DTBs that don't have an >>>>> of the multiple queue properties. >>>>> >>>>> See below... >>>>> >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> index 04d9245..5a0a781 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { >>>>>> int (*rx_ipc)(struct mac_device_info *hw); >>>>>> /* Enable RX Queues */ >>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >>>>>> + /* Program RX Algorithms */ >>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >>>>>> + /* Program TX Algorithms */ >>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >>>>>> /* Dump MAC registers */ >>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >>>>>> /* 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 db45134..748ab6f 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>> @@ -161,6 +161,16 @@ enum power_event { >>>>>> #define GMAC_HI_REG_AE BIT(31) >>>>>> >>>>>> /* MTL registers */ >>>>>> +#define MTL_OPERATION_MODE 0x00000c00 >>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >>>>>> +#define MTL_OPERATION_RAA BIT(2) >>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >>>>>> + >>>>>> #define MTL_INT_STATUS 0x00000c20 >>>>>> #define MTL_INT_Q0 BIT(0) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> index 1e79e65..f966755 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); >>>>>> } >>>>>> >>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >>>>>> + u32 rx_alg) >>>>>> +{ >>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>> + >>>>>> + value &= ~MTL_OPERATION_RAA; >>>>>> + switch (rx_alg) { >>>>>> + case MTL_RX_ALGORITHM_SP: >>>>>> + value |= MTL_OPERATION_RAA_SP; >>>>>> + break; >>>>>> + case MTL_RX_ALGORITHM_WSP: >>>>>> + value |= MTL_OPERATION_RAA_WSP; >>>>>> + break; >>>>>> + default: >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); >>>>>> +} >>>>>> + >>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >>>>>> + u32 tx_alg) >>>>>> +{ >>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>> + >>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; >>>>>> + switch (tx_alg) { >>>>>> + case MTL_TX_ALGORITHM_WRR: >>>>>> + value |= MTL_OPERATION_SCHALG_WRR; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_WFQ: >>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_DWRR: >>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_SP: >>>>>> + value |= MTL_OPERATION_SCHALG_SP; >>>>>> + break; >>>>>> + default: >>>>>> + break; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >>>>>> { >>>>>> void __iomem *ioaddr = hw->pcsr; >>>>>> @@ -457,6 +503,8 @@ 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, >>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >>>>>> .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 4498a38..af57f8d 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >>>>>> } >>>>>> >>>>>> /** >>>>>> + * stmmac_mtl_configuration - Configure MTL >>>>>> + * @priv: driver private structure >>>>>> + * Description: It is used for configurring MTL >>>>>> + */ >>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >>>>>> +{ >>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >>>>>> + >>>>>> + /* Configure MTL RX algorithms */ >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >>>>>> + priv->plat->rx_sched_algorithm); >>>>>> + >>>>>> + /* Configure MTL TX algorithms */ >>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >>>>>> + priv->plat->tx_sched_algorithm); >>>>>> + >>>>>> + /* Enable MAC RX Queues */ >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >>>>>> + stmmac_mac_enable_rx_queues(priv); >>>>> >>>>> This is almost equivalent to the code removed from stmmac_hw_setup() >>>>> which happens to be the key for this driver to work for me. However, the >>>>> code above adds an additional check for rx_queues_count > 1 which is >>>>> going to be false for any existing DTB, because it is derived from the >>>>> values retrieved from new device tree properties. >>>>> >>>>> So I think for backwards compatibility we'd need something like this: >>>>> >>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && >>>>> priv->hw->mac->rx_queue_enable) >>>>> >>>>> But then I'm beginning to think maybe we don't need a check here at all >>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and >>>>> I think it would still be legitimate to set it up even then. >>>>> >>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that >>>>> is derived from the number of RX queues derived from the feature >>>>> registers and therefore refers to the number of queues that the hardware >>>>> supports as opposed to the number of queues configured in device tree. >>>>> >>>>> I can follow up with a patch to restore backwards-compatibility. >>>> >>>> Forhw configured as single queue you don't need to enable the rx queue, since >>>> they are enable by default. if you check in stmmac_platform.c I assure backward >>>> compatibility by setting the number of rx and tx queues = 1 if nothing is >>>> declared. Please check here: >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= >>> >>> Ah yes, I just ran across that trying to debug why a subsequent patch >>> broke things again. I think the rx_queue_count > 1 condition is still >>> wrong in the above because it will still fail for the backwards- >>> compatibility case. >> >> Enabling RX Queue is needed only if the hw is configured as having more than 1 >> RX queue. If your hardware is configured as being multiple-queue, >> rx_queue_to_use should be > 1. True single queue hardware will work fine. > > But that's certainly not the case right now. The rx_queues_to_use field > is initialized based on the new snps,rx-queues-to-use device tree > property, or initialized to 1 in the absence of that property (and for > PCI). Effectively that means all users of this driver will have their > rx_queues_to_use field set to 1 until the DTS files are updated to set > some other value. But that's not a requirement that we can impose. We > still need to be able to deal with old device trees without regression. > > Even without the backwards-compatibility argument, wouldn't it make > sense to allow users to restrict usage to a single RX queue even on > hardware that supports multiple ones? It certainly works fine in my > case, I don't see any advantage in forbidding it. Corentin spotted the real problem! By mistake I restricted dma_op_mode configuration to IP Cores Version >= 4.00. Thank you Corentin for spotting that one! Do you want to make the fix or do you want me to do it and put the reported-by tag? > > Thierry >
Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: > On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: >> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: >>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: >>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: >>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >>>>>> This patch adds the RX and TX scheduling algorithms programming. >>>>>> It introduces the multiple queues configuration function >>>>>> (stmmac_mtl_configuration) in stmmac_main. >>>>>> >>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>>>> --- >>>>>> Changes v4->v5: >>>>>> - patch title update (stmicro replaced by stmmac) >>>>>> Changes v3->v4: >>>>>> - Just to keep up with patch-set version >>>>>> Changes v2->v3: >>>>>> - Switch statements with a tab >>>>>> Changes v1->v2: >>>>>> - Just to keep up with patch-set version >>>>>> >>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) >>>>> >>>>> This patch breaks backwards-compatibility with DTBs that don't have an >>>>> of the multiple queue properties. >>>>> >>>>> See below... >>>>> >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> index 04d9245..5a0a781 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { >>>>>> int (*rx_ipc)(struct mac_device_info *hw); >>>>>> /* Enable RX Queues */ >>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >>>>>> + /* Program RX Algorithms */ >>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >>>>>> + /* Program TX Algorithms */ >>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >>>>>> /* Dump MAC registers */ >>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >>>>>> /* 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 db45134..748ab6f 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>> @@ -161,6 +161,16 @@ enum power_event { >>>>>> #define GMAC_HI_REG_AE BIT(31) >>>>>> >>>>>> /* MTL registers */ >>>>>> +#define MTL_OPERATION_MODE 0x00000c00 >>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >>>>>> +#define MTL_OPERATION_RAA BIT(2) >>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >>>>>> + >>>>>> #define MTL_INT_STATUS 0x00000c20 >>>>>> #define MTL_INT_Q0 BIT(0) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> index 1e79e65..f966755 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); >>>>>> } >>>>>> >>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >>>>>> + u32 rx_alg) >>>>>> +{ >>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>> + >>>>>> + value &= ~MTL_OPERATION_RAA; >>>>>> + switch (rx_alg) { >>>>>> + case MTL_RX_ALGORITHM_SP: >>>>>> + value |= MTL_OPERATION_RAA_SP; >>>>>> + break; >>>>>> + case MTL_RX_ALGORITHM_WSP: >>>>>> + value |= MTL_OPERATION_RAA_WSP; >>>>>> + break; >>>>>> + default: >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); >>>>>> +} >>>>>> + >>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >>>>>> + u32 tx_alg) >>>>>> +{ >>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>> + >>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; >>>>>> + switch (tx_alg) { >>>>>> + case MTL_TX_ALGORITHM_WRR: >>>>>> + value |= MTL_OPERATION_SCHALG_WRR; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_WFQ: >>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_DWRR: >>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; >>>>>> + break; >>>>>> + case MTL_TX_ALGORITHM_SP: >>>>>> + value |= MTL_OPERATION_SCHALG_SP; >>>>>> + break; >>>>>> + default: >>>>>> + break; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >>>>>> { >>>>>> void __iomem *ioaddr = hw->pcsr; >>>>>> @@ -457,6 +503,8 @@ 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, >>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >>>>>> .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 4498a38..af57f8d 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >>>>>> } >>>>>> >>>>>> /** >>>>>> + * stmmac_mtl_configuration - Configure MTL >>>>>> + * @priv: driver private structure >>>>>> + * Description: It is used for configurring MTL >>>>>> + */ >>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >>>>>> +{ >>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >>>>>> + >>>>>> + /* Configure MTL RX algorithms */ >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >>>>>> + priv->plat->rx_sched_algorithm); >>>>>> + >>>>>> + /* Configure MTL TX algorithms */ >>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >>>>>> + priv->plat->tx_sched_algorithm); >>>>>> + >>>>>> + /* Enable MAC RX Queues */ >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >>>>>> + stmmac_mac_enable_rx_queues(priv); >>>>> >>>>> This is almost equivalent to the code removed from stmmac_hw_setup() >>>>> which happens to be the key for this driver to work for me. However, the >>>>> code above adds an additional check for rx_queues_count > 1 which is >>>>> going to be false for any existing DTB, because it is derived from the >>>>> values retrieved from new device tree properties. >>>>> >>>>> So I think for backwards compatibility we'd need something like this: >>>>> >>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && >>>>> priv->hw->mac->rx_queue_enable) >>>>> >>>>> But then I'm beginning to think maybe we don't need a check here at all >>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and >>>>> I think it would still be legitimate to set it up even then. >>>>> >>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that >>>>> is derived from the number of RX queues derived from the feature >>>>> registers and therefore refers to the number of queues that the hardware >>>>> supports as opposed to the number of queues configured in device tree. >>>>> >>>>> I can follow up with a patch to restore backwards-compatibility. >>>> >>>> Forhw configured as single queue you don't need to enable the rx queue, since >>>> they are enable by default. if you check in stmmac_platform.c I assure backward >>>> compatibility by setting the number of rx and tx queues = 1 if nothing is >>>> declared. Please check here: >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= >>> >>> Ah yes, I just ran across that trying to debug why a subsequent patch >>> broke things again. I think the rx_queue_count > 1 condition is still >>> wrong in the above because it will still fail for the backwards- >>> compatibility case. >> >> Enabling RX Queue is needed only if the hw is configured as having more than 1 >> RX queue. If your hardware is configured as being multiple-queue, >> rx_queue_to_use should be > 1. True single queue hardware will work fine. > > But that's certainly not the case right now. The rx_queues_to_use field > is initialized based on the new snps,rx-queues-to-use device tree > property, or initialized to 1 in the absence of that property (and for > PCI). Effectively that means all users of this driver will have their > rx_queues_to_use field set to 1 until the DTS files are updated to set > some other value. But that's not a requirement that we can impose. We > still need to be able to deal with old device trees without regression. > > Even without the backwards-compatibility argument, wouldn't it make > sense to allow users to restrict usage to a single RX queue even on > hardware that supports multiple ones? It certainly works fine in my > case, I don't see any advantage in forbidding it. Well lets see, stmmac works nowadays with a single queue and in the future it will remain workign like that for people that don't update the DT, so I see stability, not forbiding. Of course we could always assume that RX queues = 8 and TX queues = 8 which is the max for the IP, it would be much easy, but in my opinion multiple queues should be used for now by choice since it is a recent added feature. Joao > > Thierry >
On Tue, Mar 21, 2017 at 02:10:47PM +0000, Joao Pinto wrote: > ++Adding Corentin > > Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: > > On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: > >> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: > >>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: > >>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > >>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > >>>>>> This patch adds the RX and TX scheduling algorithms programming. > >>>>>> It introduces the multiple queues configuration function > >>>>>> (stmmac_mtl_configuration) in stmmac_main. > >>>>>> > >>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >>>>>> --- > >>>>>> Changes v4->v5: > >>>>>> - patch title update (stmicro replaced by stmmac) > >>>>>> Changes v3->v4: > >>>>>> - Just to keep up with patch-set version > >>>>>> Changes v2->v3: > >>>>>> - Switch statements with a tab > >>>>>> Changes v1->v2: > >>>>>> - Just to keep up with patch-set version > >>>>>> > >>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > >>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) > >>>>> > >>>>> This patch breaks backwards-compatibility with DTBs that don't have an > >>>>> of the multiple queue properties. > >>>>> > >>>>> See below... > >>>>> > >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> index 04d9245..5a0a781 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { > >>>>>> int (*rx_ipc)(struct mac_device_info *hw); > >>>>>> /* Enable RX Queues */ > >>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > >>>>>> + /* Program RX Algorithms */ > >>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > >>>>>> + /* Program TX Algorithms */ > >>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > >>>>>> /* Dump MAC registers */ > >>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > >>>>>> /* 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 db45134..748ab6f 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>> @@ -161,6 +161,16 @@ enum power_event { > >>>>>> #define GMAC_HI_REG_AE BIT(31) > >>>>>> > >>>>>> /* MTL registers */ > >>>>>> +#define MTL_OPERATION_MODE 0x00000c00 > >>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > >>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > >>>>>> +#define MTL_OPERATION_RAA BIT(2) > >>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) > >>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > >>>>>> + > >>>>>> #define MTL_INT_STATUS 0x00000c20 > >>>>>> #define MTL_INT_Q0 BIT(0) > >>>>>> > >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> index 1e79e65..f966755 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > >>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); > >>>>>> } > >>>>>> > >>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > >>>>>> + u32 rx_alg) > >>>>>> +{ > >>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>> + > >>>>>> + value &= ~MTL_OPERATION_RAA; > >>>>>> + switch (rx_alg) { > >>>>>> + case MTL_RX_ALGORITHM_SP: > >>>>>> + value |= MTL_OPERATION_RAA_SP; > >>>>>> + break; > >>>>>> + case MTL_RX_ALGORITHM_WSP: > >>>>>> + value |= MTL_OPERATION_RAA_WSP; > >>>>>> + break; > >>>>>> + default: > >>>>>> + break; > >>>>>> + } > >>>>>> + > >>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); > >>>>>> +} > >>>>>> + > >>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > >>>>>> + u32 tx_alg) > >>>>>> +{ > >>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>> + > >>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; > >>>>>> + switch (tx_alg) { > >>>>>> + case MTL_TX_ALGORITHM_WRR: > >>>>>> + value |= MTL_OPERATION_SCHALG_WRR; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_WFQ: > >>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_DWRR: > >>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_SP: > >>>>>> + value |= MTL_OPERATION_SCHALG_SP; > >>>>>> + break; > >>>>>> + default: > >>>>>> + break; > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > >>>>>> { > >>>>>> void __iomem *ioaddr = hw->pcsr; > >>>>>> @@ -457,6 +503,8 @@ 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, > >>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > >>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > >>>>>> .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 4498a38..af57f8d 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > >>>>>> } > >>>>>> > >>>>>> /** > >>>>>> + * stmmac_mtl_configuration - Configure MTL > >>>>>> + * @priv: driver private structure > >>>>>> + * Description: It is used for configurring MTL > >>>>>> + */ > >>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > >>>>>> +{ > >>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; > >>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; > >>>>>> + > >>>>>> + /* Configure MTL RX algorithms */ > >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > >>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > >>>>>> + priv->plat->rx_sched_algorithm); > >>>>>> + > >>>>>> + /* Configure MTL TX algorithms */ > >>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > >>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > >>>>>> + priv->plat->tx_sched_algorithm); > >>>>>> + > >>>>>> + /* Enable MAC RX Queues */ > >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > >>>>>> + stmmac_mac_enable_rx_queues(priv); > >>>>> > >>>>> This is almost equivalent to the code removed from stmmac_hw_setup() > >>>>> which happens to be the key for this driver to work for me. However, the > >>>>> code above adds an additional check for rx_queues_count > 1 which is > >>>>> going to be false for any existing DTB, because it is derived from the > >>>>> values retrieved from new device tree properties. > >>>>> > >>>>> So I think for backwards compatibility we'd need something like this: > >>>>> > >>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && > >>>>> priv->hw->mac->rx_queue_enable) > >>>>> > >>>>> But then I'm beginning to think maybe we don't need a check here at all > >>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and > >>>>> I think it would still be legitimate to set it up even then. > >>>>> > >>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > >>>>> is derived from the number of RX queues derived from the feature > >>>>> registers and therefore refers to the number of queues that the hardware > >>>>> supports as opposed to the number of queues configured in device tree. > >>>>> > >>>>> I can follow up with a patch to restore backwards-compatibility. > >>>> > >>>> Forhw configured as single queue you don't need to enable the rx queue, since > >>>> they are enable by default. if you check in stmmac_platform.c I assure backward > >>>> compatibility by setting the number of rx and tx queues = 1 if nothing is > >>>> declared. Please check here: > >>>> > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= > >>> > >>> Ah yes, I just ran across that trying to debug why a subsequent patch > >>> broke things again. I think the rx_queue_count > 1 condition is still > >>> wrong in the above because it will still fail for the backwards- > >>> compatibility case. > >> > >> Enabling RX Queue is needed only if the hw is configured as having more than 1 > >> RX queue. If your hardware is configured as being multiple-queue, > >> rx_queue_to_use should be > 1. True single queue hardware will work fine. > > > > But that's certainly not the case right now. The rx_queues_to_use field > > is initialized based on the new snps,rx-queues-to-use device tree > > property, or initialized to 1 in the absence of that property (and for > > PCI). Effectively that means all users of this driver will have their > > rx_queues_to_use field set to 1 until the DTS files are updated to set > > some other value. But that's not a requirement that we can impose. We > > still need to be able to deal with old device trees without regression. > > > > Even without the backwards-compatibility argument, wouldn't it make > > sense to allow users to restrict usage to a single RX queue even on > > hardware that supports multiple ones? It certainly works fine in my > > case, I don't see any advantage in forbidding it. > > Corentin spotted the real problem! By mistake I restricted dma_op_mode > configuration to IP Cores Version >= 4.00. Thank you Corentin for spotting that > one! Do you want to make the fix or do you want me to do it and put the > reported-by tag? reported-by would be enougth. But I found also that rx_queues_to_use/tx_queues_to_use are not 1 by default. The following code exit stmmac_mtl_setup() before rx_queues_to_use/tx_queues_to_use are set. rx_node = of_parse_phandle(pdev->dev.of_node, "snps,mtl-rx-config", 0); The default value must be set elsewhere.
Às 2:23 PM de 3/21/2017, Corentin Labbe escreveu: > On Tue, Mar 21, 2017 at 02:10:47PM +0000, Joao Pinto wrote: >> ++Adding Corentin >> >> Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: >>> On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: >>>> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: >>>>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: >>>>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: >>>>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >>>>>>>> This patch adds the RX and TX scheduling algorithms programming. >>>>>>>> It introduces the multiple queues configuration function >>>>>>>> (stmmac_mtl_configuration) in stmmac_main. >>>>>>>> >>>>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>>>>>> --- >>>>>>>> Changes v4->v5: >>>>>>>> - patch title update (stmicro replaced by stmmac) >>>>>>>> Changes v3->v4: >>>>>>>> - Just to keep up with patch-set version >>>>>>>> Changes v2->v3: >>>>>>>> - Switch statements with a tab >>>>>>>> Changes v1->v2: >>>>>>>> - Just to keep up with patch-set version >>>>>>>> >>>>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >>>>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >>>>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> This patch breaks backwards-compatibility with DTBs that don't have an >>>>>>> of the multiple queue properties. >>>>>>> >>>>>>> See below... >>>>>>> >>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>> index 04d9245..5a0a781 100644 >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { >>>>>>>> int (*rx_ipc)(struct mac_device_info *hw); >>>>>>>> /* Enable RX Queues */ >>>>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >>>>>>>> + /* Program RX Algorithms */ >>>>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >>>>>>>> + /* Program TX Algorithms */ >>>>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >>>>>>>> /* Dump MAC registers */ >>>>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >>>>>>>> /* 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 db45134..748ab6f 100644 >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>>>> @@ -161,6 +161,16 @@ enum power_event { >>>>>>>> #define GMAC_HI_REG_AE BIT(31) >>>>>>>> >>>>>>>> /* MTL registers */ >>>>>>>> +#define MTL_OPERATION_MODE 0x00000c00 >>>>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >>>>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >>>>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >>>>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >>>>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >>>>>>>> +#define MTL_OPERATION_RAA BIT(2) >>>>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >>>>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >>>>>>>> + >>>>>>>> #define MTL_INT_STATUS 0x00000c20 >>>>>>>> #define MTL_INT_Q0 BIT(0) >>>>>>>> >>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>> index 1e79e65..f966755 100644 >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >>>>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); >>>>>>>> } >>>>>>>> >>>>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >>>>>>>> + u32 rx_alg) >>>>>>>> +{ >>>>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>>>> + >>>>>>>> + value &= ~MTL_OPERATION_RAA; >>>>>>>> + switch (rx_alg) { >>>>>>>> + case MTL_RX_ALGORITHM_SP: >>>>>>>> + value |= MTL_OPERATION_RAA_SP; >>>>>>>> + break; >>>>>>>> + case MTL_RX_ALGORITHM_WSP: >>>>>>>> + value |= MTL_OPERATION_RAA_WSP; >>>>>>>> + break; >>>>>>>> + default: >>>>>>>> + break; >>>>>>>> + } >>>>>>>> + >>>>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >>>>>>>> + u32 tx_alg) >>>>>>>> +{ >>>>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>>>> + >>>>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; >>>>>>>> + switch (tx_alg) { >>>>>>>> + case MTL_TX_ALGORITHM_WRR: >>>>>>>> + value |= MTL_OPERATION_SCHALG_WRR; >>>>>>>> + break; >>>>>>>> + case MTL_TX_ALGORITHM_WFQ: >>>>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; >>>>>>>> + break; >>>>>>>> + case MTL_TX_ALGORITHM_DWRR: >>>>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; >>>>>>>> + break; >>>>>>>> + case MTL_TX_ALGORITHM_SP: >>>>>>>> + value |= MTL_OPERATION_SCHALG_SP; >>>>>>>> + break; >>>>>>>> + default: >>>>>>>> + break; >>>>>>>> + } >>>>>>>> +} >>>>>>>> + >>>>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >>>>>>>> { >>>>>>>> void __iomem *ioaddr = hw->pcsr; >>>>>>>> @@ -457,6 +503,8 @@ 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, >>>>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >>>>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >>>>>>>> .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 4498a38..af57f8d 100644 >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> + * stmmac_mtl_configuration - Configure MTL >>>>>>>> + * @priv: driver private structure >>>>>>>> + * Description: It is used for configurring MTL >>>>>>>> + */ >>>>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >>>>>>>> +{ >>>>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >>>>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >>>>>>>> + >>>>>>>> + /* Configure MTL RX algorithms */ >>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >>>>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >>>>>>>> + priv->plat->rx_sched_algorithm); >>>>>>>> + >>>>>>>> + /* Configure MTL TX algorithms */ >>>>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >>>>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >>>>>>>> + priv->plat->tx_sched_algorithm); >>>>>>>> + >>>>>>>> + /* Enable MAC RX Queues */ >>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >>>>>>>> + stmmac_mac_enable_rx_queues(priv); >>>>>>> >>>>>>> This is almost equivalent to the code removed from stmmac_hw_setup() >>>>>>> which happens to be the key for this driver to work for me. However, the >>>>>>> code above adds an additional check for rx_queues_count > 1 which is >>>>>>> going to be false for any existing DTB, because it is derived from the >>>>>>> values retrieved from new device tree properties. >>>>>>> >>>>>>> So I think for backwards compatibility we'd need something like this: >>>>>>> >>>>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && >>>>>>> priv->hw->mac->rx_queue_enable) >>>>>>> >>>>>>> But then I'm beginning to think maybe we don't need a check here at all >>>>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and >>>>>>> I think it would still be legitimate to set it up even then. >>>>>>> >>>>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that >>>>>>> is derived from the number of RX queues derived from the feature >>>>>>> registers and therefore refers to the number of queues that the hardware >>>>>>> supports as opposed to the number of queues configured in device tree. >>>>>>> >>>>>>> I can follow up with a patch to restore backwards-compatibility. >>>>>> >>>>>> Forhw configured as single queue you don't need to enable the rx queue, since >>>>>> they are enable by default. if you check in stmmac_platform.c I assure backward >>>>>> compatibility by setting the number of rx and tx queues = 1 if nothing is >>>>>> declared. Please check here: >>>>>> >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= >>>>> >>>>> Ah yes, I just ran across that trying to debug why a subsequent patch >>>>> broke things again. I think the rx_queue_count > 1 condition is still >>>>> wrong in the above because it will still fail for the backwards- >>>>> compatibility case. >>>> >>>> Enabling RX Queue is needed only if the hw is configured as having more than 1 >>>> RX queue. If your hardware is configured as being multiple-queue, >>>> rx_queue_to_use should be > 1. True single queue hardware will work fine. >>> >>> But that's certainly not the case right now. The rx_queues_to_use field >>> is initialized based on the new snps,rx-queues-to-use device tree >>> property, or initialized to 1 in the absence of that property (and for >>> PCI). Effectively that means all users of this driver will have their >>> rx_queues_to_use field set to 1 until the DTS files are updated to set >>> some other value. But that's not a requirement that we can impose. We >>> still need to be able to deal with old device trees without regression. >>> >>> Even without the backwards-compatibility argument, wouldn't it make >>> sense to allow users to restrict usage to a single RX queue even on >>> hardware that supports multiple ones? It certainly works fine in my >>> case, I don't see any advantage in forbidding it. >> >> Corentin spotted the real problem! By mistake I restricted dma_op_mode >> configuration to IP Cores Version >= 4.00. Thank you Corentin for spotting that >> one! Do you want to make the fix or do you want me to do it and put the >> reported-by tag? > > reported-by would be enougth. > > But I found also that rx_queues_to_use/tx_queues_to_use are not 1 by default. > > The following code exit stmmac_mtl_setup() before rx_queues_to_use/tx_queues_to_use are set. > rx_node = of_parse_phandle(pdev->dev.of_node, "snps,mtl-rx-config", 0); > > The default value must be set elsewhere. > Yes you are right, I just have a pci setup, so I was not able to test it sorry. I will fix that and add you to the thread. Thanks for checking that! Joao
On Tue, Mar 21, 2017 at 02:25:15PM +0000, Joao Pinto wrote: > Às 2:23 PM de 3/21/2017, Corentin Labbe escreveu: > > On Tue, Mar 21, 2017 at 02:10:47PM +0000, Joao Pinto wrote: > >> ++Adding Corentin > >> > >> Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: > >>> On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: > >>>> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: > >>>>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: > >>>>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > >>>>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > >>>>>>>> This patch adds the RX and TX scheduling algorithms programming. > >>>>>>>> It introduces the multiple queues configuration function > >>>>>>>> (stmmac_mtl_configuration) in stmmac_main. > >>>>>>>> > >>>>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >>>>>>>> --- > >>>>>>>> Changes v4->v5: > >>>>>>>> - patch title update (stmicro replaced by stmmac) > >>>>>>>> Changes v3->v4: > >>>>>>>> - Just to keep up with patch-set version > >>>>>>>> Changes v2->v3: > >>>>>>>> - Switch statements with a tab > >>>>>>>> Changes v1->v2: > >>>>>>>> - Just to keep up with patch-set version > >>>>>>>> > >>>>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > >>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > >>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > >>>>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > >>>>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> This patch breaks backwards-compatibility with DTBs that don't have an > >>>>>>> of the multiple queue properties. > >>>>>>> > >>>>>>> See below... > >>>>>>> > >>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>>>> index 04d9245..5a0a781 100644 > >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { > >>>>>>>> int (*rx_ipc)(struct mac_device_info *hw); > >>>>>>>> /* Enable RX Queues */ > >>>>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > >>>>>>>> + /* Program RX Algorithms */ > >>>>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > >>>>>>>> + /* Program TX Algorithms */ > >>>>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > >>>>>>>> /* Dump MAC registers */ > >>>>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > >>>>>>>> /* 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 db45134..748ab6f 100644 > >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>>>> @@ -161,6 +161,16 @@ enum power_event { > >>>>>>>> #define GMAC_HI_REG_AE BIT(31) > >>>>>>>> > >>>>>>>> /* MTL registers */ > >>>>>>>> +#define MTL_OPERATION_MODE 0x00000c00 > >>>>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > >>>>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > >>>>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > >>>>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > >>>>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > >>>>>>>> +#define MTL_OPERATION_RAA BIT(2) > >>>>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) > >>>>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > >>>>>>>> + > >>>>>>>> #define MTL_INT_STATUS 0x00000c20 > >>>>>>>> #define MTL_INT_Q0 BIT(0) > >>>>>>>> > >>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>>>> index 1e79e65..f966755 100644 > >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > >>>>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); > >>>>>>>> } > >>>>>>>> > >>>>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > >>>>>>>> + u32 rx_alg) > >>>>>>>> +{ > >>>>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>>>> + > >>>>>>>> + value &= ~MTL_OPERATION_RAA; > >>>>>>>> + switch (rx_alg) { > >>>>>>>> + case MTL_RX_ALGORITHM_SP: > >>>>>>>> + value |= MTL_OPERATION_RAA_SP; > >>>>>>>> + break; > >>>>>>>> + case MTL_RX_ALGORITHM_WSP: > >>>>>>>> + value |= MTL_OPERATION_RAA_WSP; > >>>>>>>> + break; > >>>>>>>> + default: > >>>>>>>> + break; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > >>>>>>>> + u32 tx_alg) > >>>>>>>> +{ > >>>>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>>>> + > >>>>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; > >>>>>>>> + switch (tx_alg) { > >>>>>>>> + case MTL_TX_ALGORITHM_WRR: > >>>>>>>> + value |= MTL_OPERATION_SCHALG_WRR; > >>>>>>>> + break; > >>>>>>>> + case MTL_TX_ALGORITHM_WFQ: > >>>>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; > >>>>>>>> + break; > >>>>>>>> + case MTL_TX_ALGORITHM_DWRR: > >>>>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; > >>>>>>>> + break; > >>>>>>>> + case MTL_TX_ALGORITHM_SP: > >>>>>>>> + value |= MTL_OPERATION_SCHALG_SP; > >>>>>>>> + break; > >>>>>>>> + default: > >>>>>>>> + break; > >>>>>>>> + } > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > >>>>>>>> { > >>>>>>>> void __iomem *ioaddr = hw->pcsr; > >>>>>>>> @@ -457,6 +503,8 @@ 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, > >>>>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > >>>>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > >>>>>>>> .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 4498a38..af57f8d 100644 > >>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > >>>>>>>> } > >>>>>>>> > >>>>>>>> /** > >>>>>>>> + * stmmac_mtl_configuration - Configure MTL > >>>>>>>> + * @priv: driver private structure > >>>>>>>> + * Description: It is used for configurring MTL > >>>>>>>> + */ > >>>>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > >>>>>>>> +{ > >>>>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; > >>>>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; > >>>>>>>> + > >>>>>>>> + /* Configure MTL RX algorithms */ > >>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > >>>>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > >>>>>>>> + priv->plat->rx_sched_algorithm); > >>>>>>>> + > >>>>>>>> + /* Configure MTL TX algorithms */ > >>>>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > >>>>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > >>>>>>>> + priv->plat->tx_sched_algorithm); > >>>>>>>> + > >>>>>>>> + /* Enable MAC RX Queues */ > >>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > >>>>>>>> + stmmac_mac_enable_rx_queues(priv); > >>>>>>> > >>>>>>> This is almost equivalent to the code removed from stmmac_hw_setup() > >>>>>>> which happens to be the key for this driver to work for me. However, the > >>>>>>> code above adds an additional check for rx_queues_count > 1 which is > >>>>>>> going to be false for any existing DTB, because it is derived from the > >>>>>>> values retrieved from new device tree properties. > >>>>>>> > >>>>>>> So I think for backwards compatibility we'd need something like this: > >>>>>>> > >>>>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && > >>>>>>> priv->hw->mac->rx_queue_enable) > >>>>>>> > >>>>>>> But then I'm beginning to think maybe we don't need a check here at all > >>>>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and > >>>>>>> I think it would still be legitimate to set it up even then. > >>>>>>> > >>>>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > >>>>>>> is derived from the number of RX queues derived from the feature > >>>>>>> registers and therefore refers to the number of queues that the hardware > >>>>>>> supports as opposed to the number of queues configured in device tree. > >>>>>>> > >>>>>>> I can follow up with a patch to restore backwards-compatibility. > >>>>>> > >>>>>> Forhw configured as single queue you don't need to enable the rx queue, since > >>>>>> they are enable by default. if you check in stmmac_platform.c I assure backward > >>>>>> compatibility by setting the number of rx and tx queues = 1 if nothing is > >>>>>> declared. Please check here: > >>>>>> > >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= > >>>>> > >>>>> Ah yes, I just ran across that trying to debug why a subsequent patch > >>>>> broke things again. I think the rx_queue_count > 1 condition is still > >>>>> wrong in the above because it will still fail for the backwards- > >>>>> compatibility case. > >>>> > >>>> Enabling RX Queue is needed only if the hw is configured as having more than 1 > >>>> RX queue. If your hardware is configured as being multiple-queue, > >>>> rx_queue_to_use should be > 1. True single queue hardware will work fine. > >>> > >>> But that's certainly not the case right now. The rx_queues_to_use field > >>> is initialized based on the new snps,rx-queues-to-use device tree > >>> property, or initialized to 1 in the absence of that property (and for > >>> PCI). Effectively that means all users of this driver will have their > >>> rx_queues_to_use field set to 1 until the DTS files are updated to set > >>> some other value. But that's not a requirement that we can impose. We > >>> still need to be able to deal with old device trees without regression. > >>> > >>> Even without the backwards-compatibility argument, wouldn't it make > >>> sense to allow users to restrict usage to a single RX queue even on > >>> hardware that supports multiple ones? It certainly works fine in my > >>> case, I don't see any advantage in forbidding it. > >> > >> Corentin spotted the real problem! By mistake I restricted dma_op_mode > >> configuration to IP Cores Version >= 4.00. Thank you Corentin for spotting that > >> one! Do you want to make the fix or do you want me to do it and put the > >> reported-by tag? > > > > reported-by would be enougth. > > > > But I found also that rx_queues_to_use/tx_queues_to_use are not 1 by default. > > > > The following code exit stmmac_mtl_setup() before rx_queues_to_use/tx_queues_to_use are set. > > rx_node = of_parse_phandle(pdev->dev.of_node, "snps,mtl-rx-config", 0); > > > > The default value must be set elsewhere. > > > > Yes you are right, I just have a pci setup, so I was not able to test it sorry. > I will fix that and add you to the thread. Thanks for checking that! I've got a set of patches that fix all these issues and that I've tested on Tegra186. Will send out soon. Thierry
Às 2:33 PM de 3/21/2017, Thierry Reding escreveu: > On Tue, Mar 21, 2017 at 02:25:15PM +0000, Joao Pinto wrote: >> Às 2:23 PM de 3/21/2017, Corentin Labbe escreveu: >>> On Tue, Mar 21, 2017 at 02:10:47PM +0000, Joao Pinto wrote: >>>> ++Adding Corentin >>>> >>>> Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: >>>>> On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: >>>>>> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: >>>>>>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: >>>>>>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: >>>>>>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: >>>>>>>>>> This patch adds the RX and TX scheduling algorithms programming. >>>>>>>>>> It introduces the multiple queues configuration function >>>>>>>>>> (stmmac_mtl_configuration) in stmmac_main. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>>>>>>>> --- >>>>>>>>>> Changes v4->v5: >>>>>>>>>> - patch title update (stmicro replaced by stmmac) >>>>>>>>>> Changes v3->v4: >>>>>>>>>> - Just to keep up with patch-set version >>>>>>>>>> Changes v2->v3: >>>>>>>>>> - Switch statements with a tab >>>>>>>>>> Changes v1->v2: >>>>>>>>>> - Just to keep up with patch-set version >>>>>>>>>> >>>>>>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ >>>>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ >>>>>>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ >>>>>>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- >>>>>>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> This patch breaks backwards-compatibility with DTBs that don't have an >>>>>>>>> of the multiple queue properties. >>>>>>>>> >>>>>>>>> See below... >>>>>>>>> >>>>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>>>> index 04d9245..5a0a781 100644 >>>>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>>>>>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { >>>>>>>>>> int (*rx_ipc)(struct mac_device_info *hw); >>>>>>>>>> /* Enable RX Queues */ >>>>>>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); >>>>>>>>>> + /* Program RX Algorithms */ >>>>>>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); >>>>>>>>>> + /* Program TX Algorithms */ >>>>>>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); >>>>>>>>>> /* Dump MAC registers */ >>>>>>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); >>>>>>>>>> /* 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 db45134..748ab6f 100644 >>>>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h >>>>>>>>>> @@ -161,6 +161,16 @@ enum power_event { >>>>>>>>>> #define GMAC_HI_REG_AE BIT(31) >>>>>>>>>> >>>>>>>>>> /* MTL registers */ >>>>>>>>>> +#define MTL_OPERATION_MODE 0x00000c00 >>>>>>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) >>>>>>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) >>>>>>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) >>>>>>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) >>>>>>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) >>>>>>>>>> +#define MTL_OPERATION_RAA BIT(2) >>>>>>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) >>>>>>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) >>>>>>>>>> + >>>>>>>>>> #define MTL_INT_STATUS 0x00000c20 >>>>>>>>>> #define MTL_INT_Q0 BIT(0) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>>>> index 1e79e65..f966755 100644 >>>>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c >>>>>>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) >>>>>>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, >>>>>>>>>> + u32 rx_alg) >>>>>>>>>> +{ >>>>>>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>>>>>> + >>>>>>>>>> + value &= ~MTL_OPERATION_RAA; >>>>>>>>>> + switch (rx_alg) { >>>>>>>>>> + case MTL_RX_ALGORITHM_SP: >>>>>>>>>> + value |= MTL_OPERATION_RAA_SP; >>>>>>>>>> + break; >>>>>>>>>> + case MTL_RX_ALGORITHM_WSP: >>>>>>>>>> + value |= MTL_OPERATION_RAA_WSP; >>>>>>>>>> + break; >>>>>>>>>> + default: >>>>>>>>>> + break; >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, >>>>>>>>>> + u32 tx_alg) >>>>>>>>>> +{ >>>>>>>>>> + void __iomem *ioaddr = hw->pcsr; >>>>>>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); >>>>>>>>>> + >>>>>>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; >>>>>>>>>> + switch (tx_alg) { >>>>>>>>>> + case MTL_TX_ALGORITHM_WRR: >>>>>>>>>> + value |= MTL_OPERATION_SCHALG_WRR; >>>>>>>>>> + break; >>>>>>>>>> + case MTL_TX_ALGORITHM_WFQ: >>>>>>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; >>>>>>>>>> + break; >>>>>>>>>> + case MTL_TX_ALGORITHM_DWRR: >>>>>>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; >>>>>>>>>> + break; >>>>>>>>>> + case MTL_TX_ALGORITHM_SP: >>>>>>>>>> + value |= MTL_OPERATION_SCHALG_SP; >>>>>>>>>> + break; >>>>>>>>>> + default: >>>>>>>>>> + break; >>>>>>>>>> + } >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) >>>>>>>>>> { >>>>>>>>>> void __iomem *ioaddr = hw->pcsr; >>>>>>>>>> @@ -457,6 +503,8 @@ 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, >>>>>>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, >>>>>>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, >>>>>>>>>> .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 4498a38..af57f8d 100644 >>>>>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> /** >>>>>>>>>> + * stmmac_mtl_configuration - Configure MTL >>>>>>>>>> + * @priv: driver private structure >>>>>>>>>> + * Description: It is used for configurring MTL >>>>>>>>>> + */ >>>>>>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) >>>>>>>>>> +{ >>>>>>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; >>>>>>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; >>>>>>>>>> + >>>>>>>>>> + /* Configure MTL RX algorithms */ >>>>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) >>>>>>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, >>>>>>>>>> + priv->plat->rx_sched_algorithm); >>>>>>>>>> + >>>>>>>>>> + /* Configure MTL TX algorithms */ >>>>>>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) >>>>>>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, >>>>>>>>>> + priv->plat->tx_sched_algorithm); >>>>>>>>>> + >>>>>>>>>> + /* Enable MAC RX Queues */ >>>>>>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) >>>>>>>>>> + stmmac_mac_enable_rx_queues(priv); >>>>>>>>> >>>>>>>>> This is almost equivalent to the code removed from stmmac_hw_setup() >>>>>>>>> which happens to be the key for this driver to work for me. However, the >>>>>>>>> code above adds an additional check for rx_queues_count > 1 which is >>>>>>>>> going to be false for any existing DTB, because it is derived from the >>>>>>>>> values retrieved from new device tree properties. >>>>>>>>> >>>>>>>>> So I think for backwards compatibility we'd need something like this: >>>>>>>>> >>>>>>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && >>>>>>>>> priv->hw->mac->rx_queue_enable) >>>>>>>>> >>>>>>>>> But then I'm beginning to think maybe we don't need a check here at all >>>>>>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and >>>>>>>>> I think it would still be legitimate to set it up even then. >>>>>>>>> >>>>>>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that >>>>>>>>> is derived from the number of RX queues derived from the feature >>>>>>>>> registers and therefore refers to the number of queues that the hardware >>>>>>>>> supports as opposed to the number of queues configured in device tree. >>>>>>>>> >>>>>>>>> I can follow up with a patch to restore backwards-compatibility. >>>>>>>> >>>>>>>> Forhw configured as single queue you don't need to enable the rx queue, since >>>>>>>> they are enable by default. if you check in stmmac_platform.c I assure backward >>>>>>>> compatibility by setting the number of rx and tx queues = 1 if nothing is >>>>>>>> declared. Please check here: >>>>>>>> >>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= >>>>>>> >>>>>>> Ah yes, I just ran across that trying to debug why a subsequent patch >>>>>>> broke things again. I think the rx_queue_count > 1 condition is still >>>>>>> wrong in the above because it will still fail for the backwards- >>>>>>> compatibility case. >>>>>> >>>>>> Enabling RX Queue is needed only if the hw is configured as having more than 1 >>>>>> RX queue. If your hardware is configured as being multiple-queue, >>>>>> rx_queue_to_use should be > 1. True single queue hardware will work fine. >>>>> >>>>> But that's certainly not the case right now. The rx_queues_to_use field >>>>> is initialized based on the new snps,rx-queues-to-use device tree >>>>> property, or initialized to 1 in the absence of that property (and for >>>>> PCI). Effectively that means all users of this driver will have their >>>>> rx_queues_to_use field set to 1 until the DTS files are updated to set >>>>> some other value. But that's not a requirement that we can impose. We >>>>> still need to be able to deal with old device trees without regression. >>>>> >>>>> Even without the backwards-compatibility argument, wouldn't it make >>>>> sense to allow users to restrict usage to a single RX queue even on >>>>> hardware that supports multiple ones? It certainly works fine in my >>>>> case, I don't see any advantage in forbidding it. >>>> >>>> Corentin spotted the real problem! By mistake I restricted dma_op_mode >>>> configuration to IP Cores Version >= 4.00. Thank you Corentin for spotting that >>>> one! Do you want to make the fix or do you want me to do it and put the >>>> reported-by tag? >>> >>> reported-by would be enougth. >>> >>> But I found also that rx_queues_to_use/tx_queues_to_use are not 1 by default. >>> >>> The following code exit stmmac_mtl_setup() before rx_queues_to_use/tx_queues_to_use are set. >>> rx_node = of_parse_phandle(pdev->dev.of_node, "snps,mtl-rx-config", 0); >>> >>> The default value must be set elsewhere. >>> >> >> Yes you are right, I just have a pci setup, so I was not able to test it sorry. >> I will fix that and add you to the thread. Thanks for checking that! > > I've got a set of patches that fix all these issues and that I've tested > on Tegra186. Will send out soon. > > Thierry > Great thanks! Joao
On Tue, Mar 21, 2017 at 02:18:59PM +0000, Joao Pinto wrote: > Às 2:08 PM de 3/21/2017, Thierry Reding escreveu: > > On Tue, Mar 21, 2017 at 01:58:36PM +0000, Joao Pinto wrote: > >> Às 12:24 PM de 3/21/2017, Thierry Reding escreveu: > >>> On Tue, Mar 21, 2017 at 12:02:03PM +0000, Joao Pinto wrote: > >>>> Às 11:58 AM de 3/21/2017, Thierry Reding escreveu: > >>>>> On Fri, Mar 10, 2017 at 06:24:52PM +0000, Joao Pinto wrote: > >>>>>> This patch adds the RX and TX scheduling algorithms programming. > >>>>>> It introduces the multiple queues configuration function > >>>>>> (stmmac_mtl_configuration) in stmmac_main. > >>>>>> > >>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >>>>>> --- > >>>>>> Changes v4->v5: > >>>>>> - patch title update (stmicro replaced by stmmac) > >>>>>> Changes v3->v4: > >>>>>> - Just to keep up with patch-set version > >>>>>> Changes v2->v3: > >>>>>> - Switch statements with a tab > >>>>>> Changes v1->v2: > >>>>>> - Just to keep up with patch-set version > >>>>>> > >>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ > >>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- > >>>>>> 4 files changed, 90 insertions(+), 3 deletions(-) > >>>>> > >>>>> This patch breaks backwards-compatibility with DTBs that don't have an > >>>>> of the multiple queue properties. > >>>>> > >>>>> See below... > >>>>> > >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> index 04d9245..5a0a781 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > >>>>>> @@ -455,6 +455,10 @@ struct stmmac_ops { > >>>>>> int (*rx_ipc)(struct mac_device_info *hw); > >>>>>> /* Enable RX Queues */ > >>>>>> void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); > >>>>>> + /* Program RX Algorithms */ > >>>>>> + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); > >>>>>> + /* Program TX Algorithms */ > >>>>>> + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > >>>>>> /* Dump MAC registers */ > >>>>>> void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > >>>>>> /* 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 db45134..748ab6f 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > >>>>>> @@ -161,6 +161,16 @@ enum power_event { > >>>>>> #define GMAC_HI_REG_AE BIT(31) > >>>>>> > >>>>>> /* MTL registers */ > >>>>>> +#define MTL_OPERATION_MODE 0x00000c00 > >>>>>> +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) > >>>>>> +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) > >>>>>> +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) > >>>>>> +#define MTL_OPERATION_RAA BIT(2) > >>>>>> +#define MTL_OPERATION_RAA_SP (0x0 << 2) > >>>>>> +#define MTL_OPERATION_RAA_WSP (0x1 << 2) > >>>>>> + > >>>>>> #define MTL_INT_STATUS 0x00000c20 > >>>>>> #define MTL_INT_Q0 BIT(0) > >>>>>> > >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> index 1e79e65..f966755 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c > >>>>>> @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) > >>>>>> writel(value, ioaddr + GMAC_RXQ_CTRL0); > >>>>>> } > >>>>>> > >>>>>> +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, > >>>>>> + u32 rx_alg) > >>>>>> +{ > >>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>> + > >>>>>> + value &= ~MTL_OPERATION_RAA; > >>>>>> + switch (rx_alg) { > >>>>>> + case MTL_RX_ALGORITHM_SP: > >>>>>> + value |= MTL_OPERATION_RAA_SP; > >>>>>> + break; > >>>>>> + case MTL_RX_ALGORITHM_WSP: > >>>>>> + value |= MTL_OPERATION_RAA_WSP; > >>>>>> + break; > >>>>>> + default: > >>>>>> + break; > >>>>>> + } > >>>>>> + > >>>>>> + writel(value, ioaddr + MTL_OPERATION_MODE); > >>>>>> +} > >>>>>> + > >>>>>> +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, > >>>>>> + u32 tx_alg) > >>>>>> +{ > >>>>>> + void __iomem *ioaddr = hw->pcsr; > >>>>>> + u32 value = readl(ioaddr + MTL_OPERATION_MODE); > >>>>>> + > >>>>>> + value &= ~MTL_OPERATION_SCHALG_MASK; > >>>>>> + switch (tx_alg) { > >>>>>> + case MTL_TX_ALGORITHM_WRR: > >>>>>> + value |= MTL_OPERATION_SCHALG_WRR; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_WFQ: > >>>>>> + value |= MTL_OPERATION_SCHALG_WFQ; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_DWRR: > >>>>>> + value |= MTL_OPERATION_SCHALG_DWRR; > >>>>>> + break; > >>>>>> + case MTL_TX_ALGORITHM_SP: > >>>>>> + value |= MTL_OPERATION_SCHALG_SP; > >>>>>> + break; > >>>>>> + default: > >>>>>> + break; > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) > >>>>>> { > >>>>>> void __iomem *ioaddr = hw->pcsr; > >>>>>> @@ -457,6 +503,8 @@ 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, > >>>>>> + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, > >>>>>> + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, > >>>>>> .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 4498a38..af57f8d 100644 > >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >>>>>> @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) > >>>>>> } > >>>>>> > >>>>>> /** > >>>>>> + * stmmac_mtl_configuration - Configure MTL > >>>>>> + * @priv: driver private structure > >>>>>> + * Description: It is used for configurring MTL > >>>>>> + */ > >>>>>> +static void stmmac_mtl_configuration(struct stmmac_priv *priv) > >>>>>> +{ > >>>>>> + u32 rx_queues_count = priv->plat->rx_queues_to_use; > >>>>>> + u32 tx_queues_count = priv->plat->tx_queues_to_use; > >>>>>> + > >>>>>> + /* Configure MTL RX algorithms */ > >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) > >>>>>> + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, > >>>>>> + priv->plat->rx_sched_algorithm); > >>>>>> + > >>>>>> + /* Configure MTL TX algorithms */ > >>>>>> + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) > >>>>>> + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, > >>>>>> + priv->plat->tx_sched_algorithm); > >>>>>> + > >>>>>> + /* Enable MAC RX Queues */ > >>>>>> + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) > >>>>>> + stmmac_mac_enable_rx_queues(priv); > >>>>> > >>>>> This is almost equivalent to the code removed from stmmac_hw_setup() > >>>>> which happens to be the key for this driver to work for me. However, the > >>>>> code above adds an additional check for rx_queues_count > 1 which is > >>>>> going to be false for any existing DTB, because it is derived from the > >>>>> values retrieved from new device tree properties. > >>>>> > >>>>> So I think for backwards compatibility we'd need something like this: > >>>>> > >>>>> if ((rx_queue_count == 0 || rx_queue_count > 1) && > >>>>> priv->hw->mac->rx_queue_enable) > >>>>> > >>>>> But then I'm beginning to think maybe we don't need a check here at all > >>>>> because it would only prevent RX queue setup for rx_queue_count == 1 and > >>>>> I think it would still be legitimate to set it up even then. > >>>>> > >>>>> stmmac_mac_enable_rx_queues() already checks for rx_count == 1, but that > >>>>> is derived from the number of RX queues derived from the feature > >>>>> registers and therefore refers to the number of queues that the hardware > >>>>> supports as opposed to the number of queues configured in device tree. > >>>>> > >>>>> I can follow up with a patch to restore backwards-compatibility. > >>>> > >>>> Forhw configured as single queue you don't need to enable the rx queue, since > >>>> they are enable by default. if you check in stmmac_platform.c I assure backward > >>>> compatibility by setting the number of rx and tx queues = 1 if nothing is > >>>> declared. Please check here: > >>>> > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_davem_net-2Dnext.git_tree_drivers_net_ethernet_stmicro_stmmac_stmmac-5Fplatform.c-23n156&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=tvYpf8cZr-Y6Zl-d7NlT2FEW7Gv5Bzs86GMIhZs4OSI&s=jA0P7-MXCVOtu262fF75EhUq5dfcDutzsYg8CAE6XQc&e= > >>> > >>> Ah yes, I just ran across that trying to debug why a subsequent patch > >>> broke things again. I think the rx_queue_count > 1 condition is still > >>> wrong in the above because it will still fail for the backwards- > >>> compatibility case. > >> > >> Enabling RX Queue is needed only if the hw is configured as having more than 1 > >> RX queue. If your hardware is configured as being multiple-queue, > >> rx_queue_to_use should be > 1. True single queue hardware will work fine. > > > > But that's certainly not the case right now. The rx_queues_to_use field > > is initialized based on the new snps,rx-queues-to-use device tree > > property, or initialized to 1 in the absence of that property (and for > > PCI). Effectively that means all users of this driver will have their > > rx_queues_to_use field set to 1 until the DTS files are updated to set > > some other value. But that's not a requirement that we can impose. We > > still need to be able to deal with old device trees without regression. > > > > Even without the backwards-compatibility argument, wouldn't it make > > sense to allow users to restrict usage to a single RX queue even on > > hardware that supports multiple ones? It certainly works fine in my > > case, I don't see any advantage in forbidding it. > > Well lets see, stmmac works nowadays with a single queue and in the future it > will remain workign like that for people that don't update the DT, so I see > stability, not forbiding. > > Of course we could always assume that RX queues = 8 and TX queues = 8 which is > the max for the IP, it would be much easy, but in my opinion multiple queues > should be used for now by choice since it is a recent added feature. I fully agree. The point I was arguing is that this is not how it currently works. However, I've got a set of patches to fix these issues and restore things on a Tegra186 (and likely also on other devices that are probably also broken at the moment). Thierry
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 04d9245..5a0a781 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -455,6 +455,10 @@ struct stmmac_ops { int (*rx_ipc)(struct mac_device_info *hw); /* Enable RX Queues */ void (*rx_queue_enable)(struct mac_device_info *hw, u32 queue); + /* Program RX Algorithms */ + void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg); + /* Program TX Algorithms */ + void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); /* Dump MAC registers */ void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); /* 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 db45134..748ab6f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h @@ -161,6 +161,16 @@ enum power_event { #define GMAC_HI_REG_AE BIT(31) /* MTL registers */ +#define MTL_OPERATION_MODE 0x00000c00 +#define MTL_OPERATION_SCHALG_MASK GENMASK(6, 5) +#define MTL_OPERATION_SCHALG_WRR (0x0 << 5) +#define MTL_OPERATION_SCHALG_WFQ (0x1 << 5) +#define MTL_OPERATION_SCHALG_DWRR (0x2 << 5) +#define MTL_OPERATION_SCHALG_SP (0x3 << 5) +#define MTL_OPERATION_RAA BIT(2) +#define MTL_OPERATION_RAA_SP (0x0 << 2) +#define MTL_OPERATION_RAA_WSP (0x1 << 2) + #define MTL_INT_STATUS 0x00000c20 #define MTL_INT_Q0 BIT(0) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 1e79e65..f966755 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -70,6 +70,52 @@ static void dwmac4_rx_queue_enable(struct mac_device_info *hw, u32 queue) writel(value, ioaddr + GMAC_RXQ_CTRL0); } +static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw, + u32 rx_alg) +{ + void __iomem *ioaddr = hw->pcsr; + u32 value = readl(ioaddr + MTL_OPERATION_MODE); + + value &= ~MTL_OPERATION_RAA; + switch (rx_alg) { + case MTL_RX_ALGORITHM_SP: + value |= MTL_OPERATION_RAA_SP; + break; + case MTL_RX_ALGORITHM_WSP: + value |= MTL_OPERATION_RAA_WSP; + break; + default: + break; + } + + writel(value, ioaddr + MTL_OPERATION_MODE); +} + +static void dwmac4_prog_mtl_tx_algorithms(struct mac_device_info *hw, + u32 tx_alg) +{ + void __iomem *ioaddr = hw->pcsr; + u32 value = readl(ioaddr + MTL_OPERATION_MODE); + + value &= ~MTL_OPERATION_SCHALG_MASK; + switch (tx_alg) { + case MTL_TX_ALGORITHM_WRR: + value |= MTL_OPERATION_SCHALG_WRR; + break; + case MTL_TX_ALGORITHM_WFQ: + value |= MTL_OPERATION_SCHALG_WFQ; + break; + case MTL_TX_ALGORITHM_DWRR: + value |= MTL_OPERATION_SCHALG_DWRR; + break; + case MTL_TX_ALGORITHM_SP: + value |= MTL_OPERATION_SCHALG_SP; + break; + default: + break; + } +} + static void dwmac4_dump_regs(struct mac_device_info *hw, u32 *reg_space) { void __iomem *ioaddr = hw->pcsr; @@ -457,6 +503,8 @@ 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, + .prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms, + .prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms, .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 4498a38..af57f8d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1645,6 +1645,31 @@ static void stmmac_init_tx_coalesce(struct stmmac_priv *priv) } /** + * stmmac_mtl_configuration - Configure MTL + * @priv: driver private structure + * Description: It is used for configurring MTL + */ +static void stmmac_mtl_configuration(struct stmmac_priv *priv) +{ + u32 rx_queues_count = priv->plat->rx_queues_to_use; + u32 tx_queues_count = priv->plat->tx_queues_to_use; + + /* Configure MTL RX algorithms */ + if (rx_queues_count > 1 && priv->hw->mac->prog_mtl_rx_algorithms) + priv->hw->mac->prog_mtl_rx_algorithms(priv->hw, + priv->plat->rx_sched_algorithm); + + /* Configure MTL TX algorithms */ + if (tx_queues_count > 1 && priv->hw->mac->prog_mtl_tx_algorithms) + priv->hw->mac->prog_mtl_tx_algorithms(priv->hw, + priv->plat->tx_sched_algorithm); + + /* Enable MAC RX Queues */ + if (rx_queues_count > 1 && priv->hw->mac->rx_queue_enable) + stmmac_mac_enable_rx_queues(priv); +} + +/** * stmmac_hw_setup - setup mac in a usable state. * @dev : pointer to the device structure. * Description: @@ -1688,9 +1713,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 */ - if (priv->hw->mac->rx_queue_enable) - stmmac_mac_enable_rx_queues(priv); + /* Initialize MTL*/ + if (priv->synopsys_id >= DWMAC_CORE_4_00) + stmmac_mtl_configuration(priv); ret = priv->hw->mac->rx_ipc(priv->hw); if (!ret) {
This patch adds the RX and TX scheduling algorithms programming. It introduces the multiple queues configuration function (stmmac_mtl_configuration) in stmmac_main. Signed-off-by: Joao Pinto <jpinto@synopsys.com> --- Changes v4->v5: - patch title update (stmicro replaced by stmmac) Changes v3->v4: - Just to keep up with patch-set version Changes v2->v3: - Switch statements with a tab Changes v1->v2: - Just to keep up with patch-set version drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++ drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 10 +++++ drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 48 +++++++++++++++++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++++++++++++-- 4 files changed, 90 insertions(+), 3 deletions(-)