| Message ID | 1437995941-5857-3-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Mugunthan V N <mugunthanvnm@ti.com> : [...] > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index d68d759..4f98537 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) > struct cpsw_priv *priv = dev_id; > > cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); > - cpdma_chan_process(priv->txch, 128); > + writel(0, &priv->wr_regs->tx_en); > + > + if (netif_running(priv->ndev)) { > + napi_schedule(&priv->napi_tx); > + return IRQ_HANDLED; > + } cpsw_ndo_stop calls napi_disable: you can remove netif_running.
On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: > Mugunthan V N <mugunthanvnm@ti.com> : > [...] >> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c >> index d68d759..4f98537 100644 >> --- a/drivers/net/ethernet/ti/cpsw.c >> +++ b/drivers/net/ethernet/ti/cpsw.c >> @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) >> struct cpsw_priv *priv = dev_id; >> >> cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); >> - cpdma_chan_process(priv->txch, 128); >> + writel(0, &priv->wr_regs->tx_en); >> + >> + if (netif_running(priv->ndev)) { >> + napi_schedule(&priv->napi_tx); >> + return IRQ_HANDLED; >> + } > > > cpsw_ndo_stop calls napi_disable: you can remove netif_running. > Hmmmm, Will fix in v2. Regards Mugunthan V N -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: > Mugunthan V N <mugunthanvnm@ti.com> : > [...] >> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c >> index d68d759..4f98537 100644 >> --- a/drivers/net/ethernet/ti/cpsw.c >> +++ b/drivers/net/ethernet/ti/cpsw.c >> @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) >> struct cpsw_priv *priv = dev_id; >> >> cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); >> - cpdma_chan_process(priv->txch, 128); >> + writel(0, &priv->wr_regs->tx_en); >> + >> + if (netif_running(priv->ndev)) { >> + napi_schedule(&priv->napi_tx); >> + return IRQ_HANDLED; >> + } > > > cpsw_ndo_stop calls napi_disable: you can remove netif_running. > This netif_running check is to find which interface is up as the interrupt is shared by both the interfaces. When first interface is down and second interface is active then napi_schedule for first interface will fail and second interface napi needs to be scheduled. So I don't think netif_running needs to be removed. Regards Mugunthan V N -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mugunthan V N <mugunthanvnm@ti.com> : > On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: > > Mugunthan V N <mugunthanvnm@ti.com> : [...] > >> @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) > >> struct cpsw_priv *priv = dev_id; > >> > >> cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); > >> - cpdma_chan_process(priv->txch, 128); > >> + writel(0, &priv->wr_regs->tx_en); > >> + > >> + if (netif_running(priv->ndev)) { > >> + napi_schedule(&priv->napi_tx); > >> + return IRQ_HANDLED; > >> + } > > > > > > cpsw_ndo_stop calls napi_disable: you can remove netif_running. > > > > This netif_running check is to find which interface is up as the > interrupt is shared by both the interfaces. When first interface is down > and second interface is active then napi_schedule for first interface > will fail and second interface napi needs to be scheduled. > > So I don't think netif_running needs to be removed. Each interface has its own napi tx (resp. rx) context: I would had expected two unconditional napi_schedule per tx (resp. rx) shared irq, not one. I'll read it again after some sleep.
On Wednesday 29 July 2015 04:00 AM, Francois Romieu wrote: > Mugunthan V N <mugunthanvnm@ti.com> : >> On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: >>> Mugunthan V N <mugunthanvnm@ti.com> : > [...] >>>> @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) >>>> struct cpsw_priv *priv = dev_id; >>>> >>>> cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); >>>> - cpdma_chan_process(priv->txch, 128); >>>> + writel(0, &priv->wr_regs->tx_en); >>>> + >>>> + if (netif_running(priv->ndev)) { >>>> + napi_schedule(&priv->napi_tx); >>>> + return IRQ_HANDLED; >>>> + } >>> >>> >>> cpsw_ndo_stop calls napi_disable: you can remove netif_running. >>> >> >> This netif_running check is to find which interface is up as the >> interrupt is shared by both the interfaces. When first interface is down >> and second interface is active then napi_schedule for first interface >> will fail and second interface napi needs to be scheduled. >> >> So I don't think netif_running needs to be removed. > > Each interface has its own napi tx (resp. rx) context: I would had expected > two unconditional napi_schedule per tx (resp. rx) shared irq, not one. > > I'll read it again after some sleep. > For each interrupt only one napi will be scheduled, when the first interface is down then only second interface napi is scheduled in both tx and rx irqs. Regards Mugunthan V N -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mugunthan V N <mugunthanvnm@ti.com> : > On Wednesday 29 July 2015 04:00 AM, Francois Romieu wrote: > > Mugunthan V N <mugunthanvnm@ti.com> : > >> On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: [...] > >>> cpsw_ndo_stop calls napi_disable: you can remove netif_running. > >>> > >> > >> This netif_running check is to find which interface is up as the > >> interrupt is shared by both the interfaces. When first interface is down > >> and second interface is active then napi_schedule for first interface > >> will fail and second interface napi needs to be scheduled. > >> > >> So I don't think netif_running needs to be removed. > > > > Each interface has its own napi tx (resp. rx) context: I would had expected > > two unconditional napi_schedule per tx (resp. rx) shared irq, not one. > > > > I'll read it again after some sleep. > > > > For each interrupt only one napi will be scheduled, when the first > interface is down then only second interface napi is scheduled in both > tx and rx irqs. Ok, I've had some hints from the "Assumptions" section at http://processors.wiki.ti.com/index.php/AM335x_CPSW_%28Ethernet%29_Driver%27s_Guide#Dual_Standalone_EMAC_mode Why does the driver create 2 rx napi contexts ? They don't run at the same time and the port demux is done in cpsw_dual_emac_src_port_detect. The driver would work the same with a single rx (resp. tx) napi context for both interfaces.
On Thursday 30 July 2015 04:27 AM, Francois Romieu wrote: > Mugunthan V N <mugunthanvnm@ti.com> : >> On Wednesday 29 July 2015 04:00 AM, Francois Romieu wrote: >>> Mugunthan V N <mugunthanvnm@ti.com> : >>>> On Tuesday 28 July 2015 02:52 AM, Francois Romieu wrote: > [...] >>>>> cpsw_ndo_stop calls napi_disable: you can remove netif_running. >>>>> >>>> >>>> This netif_running check is to find which interface is up as the >>>> interrupt is shared by both the interfaces. When first interface is down >>>> and second interface is active then napi_schedule for first interface >>>> will fail and second interface napi needs to be scheduled. >>>> >>>> So I don't think netif_running needs to be removed. >>> >>> Each interface has its own napi tx (resp. rx) context: I would had expected >>> two unconditional napi_schedule per tx (resp. rx) shared irq, not one. >>> >>> I'll read it again after some sleep. >>> >> >> For each interrupt only one napi will be scheduled, when the first >> interface is down then only second interface napi is scheduled in both >> tx and rx irqs. > > Ok, I've had some hints from the "Assumptions" section at > http://processors.wiki.ti.com/index.php/AM335x_CPSW_%28Ethernet%29_Driver%27s_Guide#Dual_Standalone_EMAC_mode > > Why does the driver create 2 rx napi contexts ? They don't run at the > same time and the port demux is done in cpsw_dual_emac_src_port_detect. > The driver would work the same with a single rx (resp. tx) napi context > for both interfaces. > The wiki you had pointed out is old design done on v3.2 and doesn't have device tree support also. In mainline Dual EMAC implementation has changed a lot. I can think of a way with one napi implementation for each rx and tx, will submit a separate patch for it. Regards Mugunthan V N -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index d68d759..4f98537 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -365,7 +365,8 @@ struct cpsw_priv { spinlock_t lock; struct platform_device *pdev; struct net_device *ndev; - struct napi_struct napi; + struct napi_struct napi_rx; + struct napi_struct napi_tx; struct device *dev; struct cpsw_platform_data data; struct cpsw_ss_regs __iomem *regs; @@ -752,13 +753,22 @@ static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id) struct cpsw_priv *priv = dev_id; cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX); - cpdma_chan_process(priv->txch, 128); + writel(0, &priv->wr_regs->tx_en); + + if (netif_running(priv->ndev)) { + napi_schedule(&priv->napi_tx); + return IRQ_HANDLED; + } priv = cpsw_get_slave_priv(priv, 1); - if (priv) - cpdma_chan_process(priv->txch, 128); + if (!priv) + return IRQ_NONE; - return IRQ_HANDLED; + if (netif_running(priv->ndev)) { + napi_schedule(&priv->napi_tx); + return IRQ_HANDLED; + } + return IRQ_NONE; } static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id) @@ -769,7 +779,7 @@ static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id) writel(0, &priv->wr_regs->rx_en); if (netif_running(priv->ndev)) { - napi_schedule(&priv->napi); + napi_schedule(&priv->napi_rx); return IRQ_HANDLED; } @@ -778,20 +788,37 @@ static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id) return IRQ_NONE; if (netif_running(priv->ndev)) { - napi_schedule(&priv->napi); + napi_schedule(&priv->napi_rx); return IRQ_HANDLED; } return IRQ_NONE; } -static int cpsw_poll(struct napi_struct *napi, int budget) +static int cpsw_tx_poll(struct napi_struct *napi_tx, int budget) +{ + struct cpsw_priv *priv = napi_to_priv(napi_tx); + int num_tx; + + num_tx = cpdma_chan_process(priv->txch, budget); + if (num_tx < budget) { + napi_complete(napi_tx); + writel(0xff, &priv->wr_regs->tx_en); + } + + if (num_tx) + cpsw_dbg(priv, intr, "poll %d tx pkts\n", num_tx); + + return num_tx; +} + +static int cpsw_rx_poll(struct napi_struct *napi_rx, int budget) { - struct cpsw_priv *priv = napi_to_priv(napi); + struct cpsw_priv *priv = napi_to_priv(napi_rx); int num_rx; num_rx = cpdma_chan_process(priv->rxch, budget); if (num_rx < budget) { - napi_complete(napi); + napi_complete(napi_rx); writel(0xff, &priv->wr_regs->rx_en); } @@ -1297,7 +1324,8 @@ static int cpsw_ndo_open(struct net_device *ndev) cpsw_set_coalesce(ndev, &coal); } - napi_enable(&priv->napi); + napi_enable(&priv->napi_rx); + napi_enable(&priv->napi_tx); cpdma_ctlr_start(priv->dma); cpsw_intr_enable(priv); @@ -1319,7 +1347,8 @@ static int cpsw_ndo_stop(struct net_device *ndev) cpsw_info(priv, ifdown, "shutting down cpsw device\n"); netif_stop_queue(priv->ndev); - napi_disable(&priv->napi); + napi_disable(&priv->napi_rx); + napi_disable(&priv->napi_tx); netif_carrier_off(priv->ndev); if (cpsw_common_res_usage_state(priv) <= 1) { @@ -2105,7 +2134,10 @@ static int cpsw_probe_dual_emac(struct platform_device *pdev, ndev->netdev_ops = &cpsw_netdev_ops; ndev->ethtool_ops = &cpsw_ethtool_ops; - netif_napi_add(ndev, &priv_sl2->napi, cpsw_poll, CPSW_POLL_WEIGHT); + netif_napi_add(ndev, &priv_sl2->napi_rx, cpsw_rx_poll, + CPSW_POLL_WEIGHT); + netif_napi_add(ndev, &priv_sl2->napi_tx, cpsw_tx_poll, + CPSW_POLL_WEIGHT); /* register the network device */ SET_NETDEV_DEV(ndev, &pdev->dev); @@ -2357,7 +2389,8 @@ static int cpsw_probe(struct platform_device *pdev) ndev->netdev_ops = &cpsw_netdev_ops; ndev->ethtool_ops = &cpsw_ethtool_ops; - netif_napi_add(ndev, &priv->napi, cpsw_poll, CPSW_POLL_WEIGHT); + netif_napi_add(ndev, &priv->napi_rx, cpsw_rx_poll, CPSW_POLL_WEIGHT); + netif_napi_add(ndev, &priv->napi_tx, cpsw_tx_poll, CPSW_POLL_WEIGHT); /* register the network device */ SET_NETDEV_DEV(ndev, &pdev->dev);
Instead of processing tx events in ISR itself, moving the tx event processing to a separate napi improves tx performance by 180 Mbps with omap2plus_defconfig. Also cleaning up rx napis by renaming to napi_rx for better understanding the code. Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> --- drivers/net/ethernet/ti/cpsw.c | 61 ++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 14 deletions(-)