À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 <jpi...@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