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
signature.asc
Description: PGP signature
