Date: Wed, 15 Apr 2009 13:20:03 -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: <1239826803.15474.48.camel@localhost.localdomain> In-Reply-To: <49E633C7.9030909@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>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-P2CbIcOK6oM3FCRmbcZ+
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
> >>
> > 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.
Sorry for the slow progress here, I appreciate your testing Andreas!
Sean
--=-P2CbIcOK6oM3FCRmbcZ+
Content-Disposition: attachment; filename="fwohci.c.diff"
Content-Type: text/x-patch; name="fwohci.c.diff"; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Index: fwohci.c
===================================================================
--- fwohci.c (revision 191002)
+++ 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));
}
@@ -324,11 +325,15 @@
OWRITE(sc, FWOHCI_INTSTATCLR, OHCI_INT_REG_FAIL);
fun = PHYDEV_RDCMD | (addr << PHYDEV_REGADDR);
OWRITE(sc, OHCI_PHYACCESS, fun);
+ bus_space_barrier(sc->bst, sc->bsh, OHCI_PHYACCESS,
+ 4, BUS_SPACE_BARRIER_WRITE);
+
for ( i = 0 ; i < MAX_RETRY ; i ++ ){
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)
@@ -426,20 +431,47 @@
static int
fwohci_probe_phy(struct fwohci_softc *sc, device_t dev)
{
- uint32_t reg, reg2;
+ uint32_t lps, reg, reg2;
+ int lps_counter = 0;
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);
+ 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);
+ }
+ /*
+ * 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 +485,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 +500,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 +516,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);
--=-P2CbIcOK6oM3FCRmbcZ+--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1239826803.15474.48.camel>