Date: Mon, 20 Apr 2009 10:29:39 -0700 From: Sean Bruno <sean.bruno@dsl-only.net> To: Andreas Tobler <andreast-list@fgznet.ch> Cc: freebsd-firewire <freebsd-firewire@freebsd.org>, scottl <scottl@freebsd.org>, Marius Strobl <marius@alchemy.franken.de> Subject: Re: fwochi.c and bus_space_barrier() Message-ID: <1240248579.29756.4.camel@localhost.localdomain> In-Reply-To: <49E7931C.8050603@fgznet.ch> References: <1239382529.21481.7.camel@localhost.localdomain> <20090411154000.GG8143@alchemy.franken.de> <1239600457.24831.8.camel@localhost.localdomain> <49E2F2FA.6000204@fgznet.ch> <1239639423.24831.85.camel@localhost.localdomain> <20090413170537.GI8143@alchemy.franken.de> <1239643406.24831.95.camel@localhost.localdomain> <20090413173528.GJ8143@alchemy.franken.de> <1239646889.24831.135.camel@localhost.localdomain> <20090414184741.GK8143@alchemy.franken.de> <49E4DF9F.1090804@fgznet.ch> <1239814413.15474.2.camel@localhost.localdomain> <49E61B4D.1050209@fgznet.ch> <1239819547.15474.5.camel@localhost.localdomain> <49E633C7.9030909@fgznet.ch> <1239826803.15474.48.camel@localhost.localdomain> <49E7931C.8050603@fgznet.ch>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Thu, 2009-04-16 at 22:20 +0200, Andreas Tobler wrote:
> Sean Bruno wrote:
> >>> You may want to retry several times. Like you pointed out in earlier
> >>> posts, this issue seems to be a race condition.
> >> Heh, now I remember, I did not speak about a race condition, but about a
> >> timing issue.
> >>
> >> If I leave the printfs away, it panics here.
> >>
> >> for (lps = 0, lps_counter = 0; !lps && lps_counter < 3; lps_counter++) {
> >> lps = (OREAD(sc, OHCI_HCCCTL) & OHCI_HCC_LPS);
> >> if (!lps) {
> >> pause("fwlps", (50 * hz + 999) / 1000);
> >> device_printf(dev, "lps not set,
> >> attempt(%d)\n", lps_cou
> >> nter);
> >> } /* else
> >> device_printf(dev, "lps(%0x) set\n", lps);*/
> >> }
> >>
> >> In my case the lps is not NULL, so we print something in the first run
> >> of the loop, this print statement is enough 'time' for the card to come
> >> up. If we leave the printf away, it is not enough time to come up for
> >> the card. Panic.
> >>
> >> This was the same thing I reported, adding a printf statement at the
> >> beginning of fwphy_rddata cures my panic.
> >>
> >> So I'd suggest to leave the lps test away and add always a pause(9), or
> >> does this cause headache on other archs?
> >>
> >> Thanks,
> >> Andreas
> >>
> >
> >
> > Ok, I think I've finally caught up to Marius (at least in this
> > situation).
> >
> > The *ACTUAL* issue is that fwochi_probe_phy() code isn't properly
> > handling the transition state from LPS==0 to LPS==1. In this period of
> > time, the internal SCLK on the firewire board may have not started yet.
> > There can be a period of time between the value of LPS==1 and the SCLK
> > actually starting.
> >
> > fwphy_rddata() appears to be *trying* to deal with this, but is
> > obviously failing.
> >
> > So "lps" has been set, but the PHY is not up yet. In order to access
> > PHY resources, we have to wait for SCLK to start(OHCI spec v1.1 table
> > 6.1).
> >
> > I believe your error is defined in the OHCI spec, Appendix A.6, PCI Bus
> > Errors. The bus error is supposed to happen! :) The driver just isn't
> > handling the error case properly.
> >
> > The proper fix is to handle the ERROR according to spec. I will work on
> > a proper solution this weekend. In the meantime, here is a patch to get
> > you by based on the pause() mechanism.
> >
Here is a different, more generic patch that seems to work for me on my
machines.
Essentially, make fwphy_rddata() responsible for catching the error and
implementing the pause(). This *should* have the same effect, unless I
don't understand what I'm doing.
I have eliminated the LPS check for now(see ifdef's) in
fwohci_probe_phy().
If this works, I will cleanup the ifdef's before I commit.
Sean
[-- Attachment #2 --]
Index: fwohci.c
===================================================================
--- fwohci.c (revision 191322)
+++ fwohci.c (working copy)
@@ -280,7 +280,8 @@
fun = (PHYDEV_WRCMD | (addr << PHYDEV_REGADDR) | (data << PHYDEV_WRDATA));
OWRITE(sc, OHCI_PHYACCESS, fun);
- DELAY(100);
+ bus_space_barrier(sc->bst, sc->bsh, OHCI_PHYACCESS,
+ 4, BUS_SPACE_BARRIER_WRITE);
return(fwphy_rddata( sc, addr));
}
@@ -316,43 +317,54 @@
fwphy_rddata(struct fwohci_softc *sc, u_int addr)
{
uint32_t fun, stat;
- u_int i, retry = 0;
+ int retry = 0;
addr &= 0xf;
+
#define MAX_RETRY 100
again:
+ /*
+ * Clear error register
+ */
OWRITE(sc, FWOHCI_INTSTATCLR, OHCI_INT_REG_FAIL);
+ /*
+ * Setup command to PHY
+ */
fun = PHYDEV_RDCMD | (addr << PHYDEV_REGADDR);
OWRITE(sc, OHCI_PHYACCESS, fun);
- for ( i = 0 ; i < MAX_RETRY ; i ++ ){
+ bus_space_barrier(sc->bst, sc->bsh, OHCI_PHYACCESS,
+ 4, BUS_SPACE_BARRIER_WRITE);
+
+ /*
+ * Read requested data
+ * from OHCI PHY
+ * If we generate an INT_REG_FAIL error
+ * from our request, most likely SCLK has
+ * not been started yet. pause() and retry.
+ * Mechanics of RDCMD and RDDONE are in
+ * ohci 1.1 Table 5-19
+ */
+ for ( retry = 0 ; retry < MAX_RETRY ; retry++ ){
+ /* Check for error */
+ stat = OREAD(sc, FWOHCI_INTSTAT);
+ if ((stat & OHCI_INT_REG_FAIL) != 0 ||
+ ((fun >> PHYDEV_REGADDR) & 0xf) != addr) {
+ if (firewire_debug)
+ device_printf(sc->fc.dev, "%s: OHCI_INT_REG_FAIL.\n", __func__);
+ pause("fwphyr", (50 * hz + 999) / 1000);
+ goto again;
+ }
fun = OREAD(sc, OHCI_PHYACCESS);
+ bus_space_barrier(sc->bst, sc->bsh, OHCI_PHYACCESS,
+ 4, BUS_SPACE_BARRIER_READ);
if ((fun & PHYDEV_RDCMD) == 0 && (fun & PHYDEV_RDDONE) != 0)
break;
- DELAY(100);
+
}
- if(i >= MAX_RETRY) {
- if (firewire_debug)
- device_printf(sc->fc.dev, "%s: failed(1).\n", __func__);
- if (++retry < MAX_RETRY) {
- DELAY(100);
- goto again;
- }
- }
- /* Make sure that SCLK is started */
- stat = OREAD(sc, FWOHCI_INTSTAT);
- if ((stat & OHCI_INT_REG_FAIL) != 0 ||
- ((fun >> PHYDEV_REGADDR) & 0xf) != addr) {
- if (firewire_debug)
- device_printf(sc->fc.dev, "%s: failed(2).\n", __func__);
- if (++retry < MAX_RETRY) {
- DELAY(100);
- goto again;
- }
- }
if (firewire_debug > 1 || retry >= MAX_RETRY)
device_printf(sc->fc.dev,
- "%s:: 0x%x loop=%d, retry=%d\n",
- __func__, addr, i, retry);
+ "%s:: 0x%x, retry=%d\n",
+ __func__, addr, retry);
#undef MAX_RETRY
return((fun >> PHYDEV_RDDATA )& 0xff);
}
@@ -426,20 +438,52 @@
static int
fwohci_probe_phy(struct fwohci_softc *sc, device_t dev)
{
+#if 0
+ uint32_t lps, reg, reg2;
+ int lps_counter = 0;
+#endif
uint32_t reg, reg2;
int e1394a = 1;
-/*
- * probe PHY parameters
- * 0. to prove PHY version, whether compliance of 1394a.
- * 1. to probe maximum speed supported by the PHY and
- * number of port supported by core-logic.
- * It is not actually available port on your PC .
- */
+
+ /*
+ * Enable LPS(Link Power Status as per
+ * section 5.7 of OHCI v1.1
+ * This allows PHY communication after
+ * a hard/soft reset
+ *
+ * Some users report that the code
+ * will crash without the pause due
+ * to the lps bit being set and the
+ * PHY not being up. Strictly speaking,
+ * this code SHOULD check that the SCLK
+ * has started before it attempts
+ * to access other PHY resources.
+ */
OWRITE(sc, OHCI_HCCCTL, OHCI_HCC_LPS);
- DELAY(500);
-
+ bus_space_barrier(sc->bst, sc->bsh, OHCI_HCCCTL, 4, BUS_SPACE_BARRIER_WRITE);
+#if 0
+ for (lps = 0, lps_counter = 0; !lps && lps_counter < 3; lps_counter++) {
+ pause("fwlps", (50 * hz + 999) / 1000);
+ lps = (OREAD(sc, OHCI_HCCCTL) & OHCI_HCC_LPS);
+ }
+#endif
+ /*
+ * probe PHY parameters
+ * 0. to prove PHY version, whether compliance of 1394a.
+ * 1. to probe maximum speed supported by the PHY and
+ * number of port supported by core-logic.
+ * It is not actually available port on your PC .
+ */
reg = fwphy_rddata(sc, FW_PHY_SPD_REG);
+ /*
+ * ref 1394-2000 table 5B-1
+ * ref 1394-1995 table J.12
+ * If Extended is not set
+ * Assume 1394-1995
+ * If Extended is set
+ * Assume 1394-2000(1394a)
+ */
if((reg >> 5) != 7 ){
sc->fc.mode &= ~FWPHYASYST;
sc->fc.nport = reg & FW_PHY_NP;
@@ -453,12 +497,12 @@
"Phy 1394 only %s, %d ports.\n",
linkspeed[sc->fc.speed], sc->fc.nport);
}else{
- reg2 = fwphy_rddata(sc, FW_PHY_ESPD_REG);
sc->fc.mode |= FWPHYASYST;
sc->fc.nport = reg & FW_PHY_NP;
+ reg2 = fwphy_rddata(sc, FW_PHY_ESPD_REG);
sc->fc.speed = (reg2 & FW_PHY_ESPD) >> 5;
if (sc->fc.speed > MAX_SPEED) {
- device_printf(dev, "invalid speed %d (fixed to %d).\n",
+ device_printf(dev, "invalid extended speed %d (fixed to %d).\n",
sc->fc.speed, MAX_SPEED);
sc->fc.speed = MAX_SPEED;
}
@@ -468,11 +512,7 @@
/* check programPhyEnable */
reg2 = fwphy_rddata(sc, 5);
-#if 0
- if (e1394a && (OREAD(sc, OHCI_HCCCTL) & OHCI_HCC_PRPHY)) {
-#else /* XXX force to enable 1394a */
if (e1394a) {
-#endif
if (firewire_debug)
device_printf(dev,
"Enable 1394a Enhancements\n");
@@ -488,6 +528,13 @@
reg2 = fwphy_wrdata(sc, 5, reg2);
}
+ /*
+ * Re-read and check for extended 1394a
+ * PHY Register map.
+ * Then set the Contender bit on.
+ * This makes us a possible bus or isoc
+ * resource manager. (ieee 1394a-2000, 5B.1)
+ */
reg = fwphy_rddata(sc, FW_PHY_SPD_REG);
if((reg >> 5) == 7 ){
reg = fwphy_rddata(sc, 4);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1240248579.29756.4.camel>