From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hein Tibosch Subject: Re: net/macb: clear tx/rx completion flags in ISR Date: Fri, 19 Apr 2013 17:21:20 +0800 Message-ID: <51710C90.5070606@yahoo.es> References: <5170D276.6070208@yahoo.es> <20130419073058.GA660@pengutronix.de> <5170F6DB.6030500@atmel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Steffen Trumtrar , netdev@vger.kernel.org, Ludovic Desroches To: Nicolas Ferre Return-path: Received: from bosmailout07.eigbox.net ([66.96.186.7]:45557 "EHLO bosmailout07.eigbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756896Ab3DSJYG (ORCPT ); Fri, 19 Apr 2013 05:24:06 -0400 Received: from bosmailscan09.eigbox.net ([10.20.15.9]) by bosmailout07.eigbox.net with esmtp (Exim) id 1UT7Y4-0000Dq-CX for netdev@vger.kernel.org; Fri, 19 Apr 2013 05:24:04 -0400 In-Reply-To: <5170F6DB.6030500@atmel.com> Sender: netdev-owner@vger.kernel.org List-ID: Hi, On 4/19/2013 3:48 PM, Nicolas Ferre wrote: > On 04/19/2013 09:30 AM, Steffen Trumtrar : >> Hi Hein, >> >> On Fri, Apr 19, 2013 at 01:13:26PM +0800, Hein Tibosch wrote: >>> Hi Steffen, >>> >>>> At least in the cadence IP core on the Xilinx Zynq SoC the TCOMP/RCOMP flags >>>> are not auto-cleaned. As these flags are evaluated, they need to be cleaned. >>> This patch does not work for at least the AVR32 platform. Both RCOMP/RCOMP >>> are cleared by *reading* the ISR and writing them would be fatal. >>> >> :-( >> >>> Could you tell me the version of the macb of Xilinx Zynq? >>> >>> u32 version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1)) >>> | MACB_GREGS_VERSION; >>> >>> On an AP7000 it reads as 0x0000010D >>> >> This gives me 0x00000119. The TRM says it is version r1p23. >> >>> I am thinking of making a patch like: >>> >>> if (bp->version >= xxx) >>> macb_writel(bp, ISR, MACB_BIT(TCOMP)); >>> >>> if (bp->version >= xxx) >>> macb_writel(bp, ISR, MACB_BIT(RCOMP)); >>> >>> which would make it work on both platforms. > Well, keep in mind that it is the hot path: It can harm the performance > if too much tests are performed... Yes it's in the hot path, both tests will be done within an interrupt. I just gave it a quick try: --- drivers/net/ethernet/cadence/macb.c | 8 ++++++-- drivers/net/ethernet/cadence/macb.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index ed2cb13..fe31951 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -315,6 +315,8 @@ int macb_mii_init(struct macb *bp) struct macb_platform_data *pdata; int err = -ENXIO, i; + bp->ip_version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1)) + | MACB_GREGS_VERSION; /* Enable management port */ macb_writel(bp, NCR, MACB_BIT(MPE)); @@ -485,7 +487,8 @@ static void macb_tx_interrupt(struct macb *bp) status = macb_readl(bp, TSR); macb_writel(bp, TSR, status); - macb_writel(bp, ISR, MACB_BIT(TCOMP)); + if (bp->ip_version == 0x00000119) + macb_writel(bp, ISR, MACB_BIT(TCOMP)); netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n", (unsigned long)status); @@ -738,7 +741,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id) * now. */ macb_writel(bp, IDR, MACB_RX_INT_FLAGS); - macb_writel(bp, ISR, MACB_BIT(RCOMP)); + if (bp->ip_version == 0x00000119) + macb_writel(bp, ISR, MACB_BIT(RCOMP)); if (napi_schedule_prep(&bp->napi)) { netdev_vdbg(bp->dev, "scheduling RX softirq\n"); diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 993d703..0fbb440 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -575,6 +575,7 @@ struct macb { unsigned int duplex; phy_interface_t phy_interface; + unsigned ip_version; /* AT91RM9200 transmit */ struct sk_buff *skb; /* holds skb until xmit interrupt completes */ -- 1.7.10 On my AP7000 platforms, on a 100mbit LAN, I could not measure any drop of performance using iperf: Before the patch: [ 6] 0.0-10.1 sec 67.4 MBytes 56.1 Mbits/sec [ 7] 0.0-10.1 sec 65.5 MBytes 54.6 Mbits/sec [ 6] 0.0-10.1 sec 67.8 MBytes 56.4 Mbits/sec [ 7] 0.0-10.1 sec 65.1 MBytes 54.3 Mbits/sec After the patch: [ 6] 0.0-10.1 sec 68.1 MBytes 56.8 Mbits/sec [ 7] 0.0-10.1 sec 66.5 MBytes 55.4 Mbits/sec [ 6] 0.0- 9.9 sec 67.5 MBytes 57.2 Mbits/sec [ 8] 0.0-10.1 sec 66.5 MBytes 55.5 Mbits/sec It looks a bit faster, which is pure coincidence. > >> The documentation I have is a little bit confusing in that regard. >> The cadence datasheet says, this register is R/W, the Xilinx datasheet says, >> it is "normaly RO", but the programming guide explicitely mentions clearing >> the bit by writing to it. Steffen, did you really see it happen that TCOMP/RCOMP were not cleared by just reading the ISR? The Atmel manual says about each ISR field: 'Cleared on read' >> It seems, that something like your patch is inevitable. > I also had bad feedbacks concerning this patch. Maybe we should take > more time to validate this change: event it is in net-next, maybe we > should revert it for the moment... Regards, Hein