Date: Tue, 6 Apr 2010 11:44:56 -0700 From: Pyun YongHyeon <pyunyh@gmail.com> To: Andre Albsmeier <Andre.Albsmeier@siemens.com> Cc: "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, Pyun YongHyeon <yongari@freebsd.org> Subject: Re: svn commit: r205614 - stable/7/sys/dev/msk Message-ID: <20100406184456.GA1087@michelle.cdnetworks.com> In-Reply-To: <20100406180027.GA3724@curry.mchp.siemens.de> References: <201003241721.o2OHL5K9063538@svn.freebsd.org> <20100405145937.GA78871@curry.mchp.siemens.de> <20100405180642.GD1225@michelle.cdnetworks.com> <20100406134626.GA1727@curry.mchp.siemens.de> <20100406180027.GA3724@curry.mchp.siemens.de>
next in thread | previous in thread | raw e-mail | index | archive | help
--45Z9DzgjV8m4Oswq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Tue, Apr 06, 2010 at 08:00:27PM +0200, Andre Albsmeier wrote:
> On Tue, 06-Apr-2010 at 15:46:26 +0200, Andre Albsmeier wrote:
> > On Mon, 05-Apr-2010 at 20:06:42 +0200, Pyun YongHyeon wrote:
[...]
> > > As you know 1.18.2.38 removed taskqueue based interrupt handling so
> > > it could be culprit of the issue. But that revision also removed
> > > two register accesses in TX path so I'd like to know which one
> > > caused the issue.
> >
> > I have now tried rev. 1.18.2.38 with this patch (no idea if
> > this is right ;-)):
> >
> > --- if_msk.c.1.18.2.38 2010-04-06 15:09:19.000000000 +0200
> > +++ if_msk.c.1.18.2.38.TRY 2010-04-06 15:38:13.000000000 +0200
> > @@ -3327,6 +3327,11 @@
> > uint32_t control, status;
> > int cons, len, port, rxprog;
> >
> > + int idx;
> > + idx = CSR_READ_2(sc, STAT_PUT_IDX);
> > + if (idx == sc->msk_stat_cons)
> > + return (0);
> > +
> > /* Sync status LEs. */
> > bus_dmamap_sync(sc->msk_stat_tag, sc->msk_stat_map,
> > BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
> > @@ -3407,7 +3412,7 @@
> > if (rxput[MSK_PORT_B] > 0)
> > msk_rxput(sc->msk_if[MSK_PORT_B]);
> >
> > - return (rxprog > sc->msk_process_limit ? EAGAIN : 0);
> > + return (sc->msk_stat_cons != CSR_READ_2(sc, STAT_PUT_IDX));
> > }
> >
> > static void
> >
> > Now performance seems to be the same as with the older
> > driver (at least here at work) and in both directions!
> > Some numbers:
> >
> > em0 writes to rev. 1.18.2.36: 20 seconds
> > em0 writes to rev. 1.18.2.38: 50 seconds
> > em0 writes to rev. 1.18.2.38 with patch from above: 23 seconds
> > same as before but with int_holdoff: 100 -> 1: 20 seconds
> >
> > rev. 1.18.2.36 writes to em0: 22 seconds
> > rev. 1.18.2.38 writes to em0: 40 seconds
> > rev. 1.18.2.38 with patch from above writes to em0: 21 seconds
> > same as before but with int_holdoff: 100 -> 1: 20 seconds
> >
> > It seems that these two CSR_READ_2s really help ;-).
> >
> > As I said, this is at work and with slightly different machines.
> > I will try things at home later but I am rather confident of
> > receiving good results there as well...
>
> OK, tests at home show similar good results with the
> above patch. When setting int_holdoff to 3, performance
> seems equal to the older versions.
>
Thanks a lot for narrowing down the issue.
Because the msk_handle_events() are called in interrupt handler
I'd like to remove the two register accesses in fast path. It seems
accessing STAT_PUT_IDX triggers status updates. Would you try
attached patch and let me know whether the patch makes any
difference?
--45Z9DzgjV8m4Oswq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="msk.status.update.patch"
Index: sys/dev/msk/if_msk.c
===================================================================
--- sys/dev/msk/if_msk.c (revision 206204)
+++ sys/dev/msk/if_msk.c (working copy)
@@ -1470,6 +1470,7 @@
/* WA for dev. #4.18 */
CSR_WRITE_1(sc, STAT_FIFO_WM, 0x21);
CSR_WRITE_1(sc, STAT_FIFO_ISR_WM, 0x07);
+ CSR_WRITE_4(sc, STAT_TX_TIMER_INI, MSK_USECS(sc, 10));
} else {
CSR_WRITE_2(sc, STAT_TX_IDX_TH, 0x0a);
CSR_WRITE_1(sc, STAT_FIFO_WM, 0x10);
@@ -1481,9 +1482,16 @@
CSR_WRITE_4(sc, STAT_ISR_TIMER_INI, 0x0190);
}
/*
- * Use default value for STAT_ISR_TIMER_INI, STAT_LEV_TIMER_INI.
+ * STAT_TX_TIMER_INI, STAT_ISR_TIMER_INI and STAT_LEV_TIMER_INI
+ * as well as various water mark registers seem to control when
+ * controller initiates status LE update.
+ * It's not clear how these registers interact with interrupt
+ * state but STAT_TX_TIMER_INI seems to control status update
+ * time after crossing a threshold(STAT_TX_IDX_TH) value. Due to
+ * the complexity of various parameters that may affect status
+ * update just use hardware default until we know better
+ * the internal things of status updates.
*/
- CSR_WRITE_4(sc, STAT_TX_TIMER_INI, MSK_USECS(sc, 1000));
/* Enable status unit. */
CSR_WRITE_4(sc, STAT_CTRL, SC_STAT_OP_ON);
--45Z9DzgjV8m4Oswq--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100406184456.GA1087>