Following in replies:
[PATCH 1/4 resend] firewire: ohci: work around cycle timer bugs on VIA controllers
[PATCH 2/4] firewire: ohci: enable cycle timer fix on ALi and NEC controllers
[PATCH 3/4] firewire: get_cycle_timer optimization and cleanup
[PATCH 4/4] firewire: remove incomplete Bus_Time CSR support
firewire/core-cdev.c | 17 +++----
firewire/core-transaction.c | 17 ++-----
firewire/core.h | 2
firewire/ohci.c | 83 ++++++++++++++++++++++++++----------
4 files changed, 75 insertions(+), 44 deletions(-)
--
Stefan Richter
-=====-==-=- --=- -===-
http://arcgraph.de/sr/
Date: Wed, 20 Jan 2010 09:58:02 +0100
From: Clemens Ladisch <[email protected]>
VIA controllers sometimes return an inconsistent value when reading the
isochronous cycle timer register. To work around this, read the
register multiple times and add consistency checks.
Signed-off-by: Clemens Ladisch <[email protected]>
Reported-by: Pieter Palmers <[email protected]>
Reported-by: H?kan Johansson <[email protected]>
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/ohci.c | 52 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 49 insertions(+), 3 deletions(-)
Index: linux-2.6.33-rc8/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/ohci.c
+++ linux-2.6.33-rc8/drivers/firewire/ohci.c
@@ -192,6 +192,7 @@ struct fw_ohci {
bool use_dualbuffer;
bool old_uninorth;
bool bus_reset_packet_quirk;
+ bool iso_cycle_timer_quirk;
/*
* Spinlock for accessing fw_ohci data. Never call out of
@@ -1794,14 +1795,57 @@ static int ohci_enable_phys_dma(struct f
#endif /* CONFIG_FIREWIRE_OHCI_REMOTE_DMA */
}
+static inline u32 cycle_timer_ticks(u32 cycle_timer)
+{
+ u32 ticks;
+
+ ticks = cycle_timer & 0xfff;
+ ticks += 3072 * ((cycle_timer >> 12) & 0x1fff);
+ ticks += (3072 * 8000) * (cycle_timer >> 25);
+ return ticks;
+}
+
static u64 ohci_get_bus_time(struct fw_card *card)
{
struct fw_ohci *ohci = fw_ohci(card);
- u32 cycle_time;
+ u32 c0, c1, c2;
+ u32 t0, t1, t2;
+ s32 diff01, diff12;
u64 bus_time;
- cycle_time = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
- bus_time = ((u64)atomic_read(&ohci->bus_seconds) << 32) | cycle_time;
+ if (!ohci->iso_cycle_timer_quirk) {
+ c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
+ } else {
+ /*
+ * VIA controllers have two bugs when updating the iso cycle
+ * timer register:
+ * 1) When the lowest six bits are wrapping around to zero,
+ * a read that happens at the same time will return garbage
+ * in the lowest ten bits.
+ * 2) When the cycleOffset field wraps around to zero, the
+ * cycleCount field is not incremented for about 60 ns.
+ *
+ * To catch these, we read the register three times and ensure
+ * that the difference between each two consecutive reads is
+ * approximately the same, i.e., less than twice the other.
+ * Furthermore, any negative difference indicates an error.
+ * (A PCI read should take at least 20 ticks of the 24.576 MHz
+ * timer to execute, so we have enough precision to compute the
+ * ratio of the differences.)
+ */
+ do {
+ c0 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
+ c1 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
+ c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
+ t0 = cycle_timer_ticks(c0);
+ t1 = cycle_timer_ticks(c1);
+ t2 = cycle_timer_ticks(c2);
+ diff01 = t1 - t0;
+ diff12 = t2 - t1;
+ } while (diff01 <= 0 || diff12 <= 0 ||
+ diff01 / diff12 >= 2 || diff12 / diff01 >= 2);
+ }
+ bus_time = ((u64)atomic_read(&ohci->bus_seconds) << 32) | c2;
return bus_time;
}
@@ -2503,6 +2547,8 @@ static int __devinit pci_probe(struct pc
#endif
ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
+ ohci->iso_cycle_timer_quirk = dev->vendor == PCI_VENDOR_ID_VIA;
+
ar_context_init(&ohci->ar_request_ctx, ohci,
OHCI1394_AsReqRcvContextControlSet);
--
Stefan Richter
-=====-==-=- --=- -===-
http://arcgraph.de/sr/
Discussed in "read_cycle_timer backwards for sub-cycle 0000, 0001",
http://thread.gmane.org/gmane.linux.kernel.firewire.devel/13704
Known bad controllers:
ALi M5271, listed by lspci as M5253 [10b9:5253]
NEC OrangeLink [1033:00cd] (rev 03)
NEC uPD72874 [1033:00f2] (rev 01)
VIA VT6306 [1106:3044] (rev 46)
VIA VT6308P, listed by lspci as rev c0
Reported-by: Pieter Palmers <[email protected]>
Reported-by: H?kan Johansson <[email protected]>
Reported-by: Clemens Ladisch <[email protected]>
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/ohci.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
Index: linux-2.6.33-rc8/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/ohci.c
+++ linux-2.6.33-rc8/drivers/firewire/ohci.c
@@ -1817,13 +1817,14 @@ static u64 ohci_get_bus_time(struct fw_c
c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
} else {
/*
- * VIA controllers have two bugs when updating the iso cycle
- * timer register:
- * 1) When the lowest six bits are wrapping around to zero,
+ * Some controllers exhibit one or more of the following bugs
+ * when updating the iso cycle timer register:
+ * - When the lowest six bits are wrapping around to zero,
* a read that happens at the same time will return garbage
* in the lowest ten bits.
- * 2) When the cycleOffset field wraps around to zero, the
+ * - When the cycleOffset field wraps around to zero, the
* cycleCount field is not incremented for about 60 ns.
+ * - Occasionally, the entire register reads zero.
*
* To catch these, we read the register three times and ensure
* that the difference between each two consecutive reads is
@@ -2547,7 +2548,9 @@ static int __devinit pci_probe(struct pc
#endif
ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
- ohci->iso_cycle_timer_quirk = dev->vendor == PCI_VENDOR_ID_VIA;
+ ohci->iso_cycle_timer_quirk = dev->vendor == PCI_VENDOR_ID_AL ||
+ dev->vendor == PCI_VENDOR_ID_NEC ||
+ dev->vendor == PCI_VENDOR_ID_VIA;
ar_context_init(&ohci->ar_request_ctx, ohci,
OHCI1394_AsReqRcvContextControlSet);
--
Stefan Richter
-=====-==-=- --=- -===-
http://arcgraph.de/sr/
ohci: Break out of the retry loop if too many attempts were necessary.
This may theoretically happen if the chip is fatally defective or if the
get_cycle_timer ioctl was performed after a CardBus controller was
ejected.
Also micro-optimize the loop by re-using the last two register reads in
the next iteration, remove a questionable inline keyword, and shuffle a
comment around.
core: ioctl_get_cycle_timer() is always called with interrupts on,
therefore local_irq_save() can be replaced by local_irq_disable().
Disabled local IRQs imply disabled preemption, hence preempt_disable()
can be removed.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/core-cdev.c | 17 ++++------
drivers/firewire/ohci.c | 57 ++++++++++++++++++-----------------
2 files changed, 36 insertions(+), 38 deletions(-)
Index: linux-2.6.33-rc8/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.33-rc8/drivers/firewire/core-cdev.c
@@ -25,6 +25,7 @@
#include <linux/firewire.h>
#include <linux/firewire-cdev.h>
#include <linux/idr.h>
+#include <linux/irqflags.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/kref.h>
@@ -32,8 +33,6 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/poll.h>
-#include <linux/preempt.h>
-#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/string.h>
#include <linux/time.h>
@@ -1035,21 +1034,19 @@ static int ioctl_get_cycle_timer(struct
{
struct fw_cdev_get_cycle_timer *request = buffer;
struct fw_card *card = client->device->card;
- unsigned long long bus_time;
struct timeval tv;
- unsigned long flags;
+ u32 cycle_time;
- preempt_disable();
- local_irq_save(flags);
+ local_irq_disable();
- bus_time = card->driver->get_bus_time(card);
+ cycle_time = card->driver->get_bus_time(card);
do_gettimeofday(&tv);
- local_irq_restore(flags);
- preempt_enable();
+ local_irq_enable();
request->local_time = tv.tv_sec * 1000000ULL + tv.tv_usec;
- request->cycle_timer = bus_time & 0xffffffff;
+ request->cycle_timer = cycle_time;
+
return 0;
}
Index: linux-2.6.33-rc8/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/ohci.c
+++ linux-2.6.33-rc8/drivers/firewire/ohci.c
@@ -1795,60 +1795,61 @@ static int ohci_enable_phys_dma(struct f
#endif /* CONFIG_FIREWIRE_OHCI_REMOTE_DMA */
}
-static inline u32 cycle_timer_ticks(u32 cycle_timer)
+static u32 cycle_timer_ticks(u32 cycle_timer)
{
u32 ticks;
ticks = cycle_timer & 0xfff;
ticks += 3072 * ((cycle_timer >> 12) & 0x1fff);
ticks += (3072 * 8000) * (cycle_timer >> 25);
+
return ticks;
}
+/*
+ * Some controllers exhibit one or more of the following bugs when updating the
+ * iso cycle timer register:
+ * - When the lowest six bits are wrapping around to zero, a read that happens
+ * at the same time will return garbage in the lowest ten bits.
+ * - When the cycleOffset field wraps around to zero, the cycleCount field is
+ * not incremented for about 60 ns.
+ * - Occasionally, the entire register reads zero.
+ *
+ * To catch these, we read the register three times and ensure that the
+ * difference between each two consecutive reads is approximately the same, i.e.
+ * less than twice the other. Furthermore, any negative difference indicates an
+ * error. (A PCI read should take at least 20 ticks of the 24.576 MHz timer to
+ * execute, so we have enough precision to compute the ratio of the differences.)
+ */
static u64 ohci_get_bus_time(struct fw_card *card)
{
struct fw_ohci *ohci = fw_ohci(card);
u32 c0, c1, c2;
u32 t0, t1, t2;
s32 diff01, diff12;
- u64 bus_time;
+ int i;
+
+ c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
- if (!ohci->iso_cycle_timer_quirk) {
+ if (ohci->iso_cycle_timer_quirk) {
+ i = 0;
+ c1 = c2;
c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
- } else {
- /*
- * Some controllers exhibit one or more of the following bugs
- * when updating the iso cycle timer register:
- * - When the lowest six bits are wrapping around to zero,
- * a read that happens at the same time will return garbage
- * in the lowest ten bits.
- * - When the cycleOffset field wraps around to zero, the
- * cycleCount field is not incremented for about 60 ns.
- * - Occasionally, the entire register reads zero.
- *
- * To catch these, we read the register three times and ensure
- * that the difference between each two consecutive reads is
- * approximately the same, i.e., less than twice the other.
- * Furthermore, any negative difference indicates an error.
- * (A PCI read should take at least 20 ticks of the 24.576 MHz
- * timer to execute, so we have enough precision to compute the
- * ratio of the differences.)
- */
do {
- c0 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
- c1 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
+ c0 = c1;
+ c1 = c2;
c2 = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
t0 = cycle_timer_ticks(c0);
t1 = cycle_timer_ticks(c1);
t2 = cycle_timer_ticks(c2);
diff01 = t1 - t0;
diff12 = t2 - t1;
- } while (diff01 <= 0 || diff12 <= 0 ||
- diff01 / diff12 >= 2 || diff12 / diff01 >= 2);
+ } while ((diff01 <= 0 || diff12 <= 0 ||
+ diff01 / diff12 >= 2 || diff12 / diff01 >= 2)
+ && i++ < 20);
}
- bus_time = ((u64)atomic_read(&ohci->bus_seconds) << 32) | c2;
- return bus_time;
+ return ((u64)atomic_read(&ohci->bus_seconds) << 32) | c2;
}
static void copy_iso_headers(struct iso_context *ctx, void *p)
--
Stefan Richter
-=====-==-=- --=- -===-
http://arcgraph.de/sr/
The current implementation of Bus_Time read access was buggy since it
did not ensure that Bus_Time.second_count_hi and second_count_lo came
from the same 128 seconds period.
Reported-by: H?kan Johansson <[email protected]>
Instead of a fix, remove Bus_Time register support altogether. The spec
requires all cycle master capable nodes to implement this (all Linux
nodes are cycle master capable) while it also says that it "may" be
initialized by the bus manager or by the IRM standing in for a bus
manager. (Neither Linux' firewire-core nor ieee1394 nodemgr implement
this.)
Since we cannot rely on Bus_Time having been initialized by a bus
manager, it is better to return an error instead of a nonsensical value
on a read request to Bus_Time.
Alternatively, we could fix the Bus_Time read integrity bug _and_
implement (a) cycle master's write support of the register as well as
(b) bus manager's Bus_Time initialization service, i.e. preservation of
the Bus_Time when the cycle master node of a bus changes. However, that
would be quite some code for a feature that is unreliable to begin with
and very likely unused in practice.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/core-cdev.c | 2 +-
drivers/firewire/core-transaction.c | 17 ++++++-----------
drivers/firewire/core.h | 2 +-
drivers/firewire/ohci.c | 25 +++++++------------------
4 files changed, 15 insertions(+), 31 deletions(-)
Index: linux-2.6.33-rc8/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.33-rc8/drivers/firewire/core-cdev.c
@@ -1039,7 +1039,7 @@ static int ioctl_get_cycle_timer(struct
local_irq_disable();
- cycle_time = card->driver->get_bus_time(card);
+ cycle_time = card->driver->get_cycle_time(card);
do_gettimeofday(&tv);
local_irq_enable();
Index: linux-2.6.33-rc8/drivers/firewire/core-transaction.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/core-transaction.c
+++ linux-2.6.33-rc8/drivers/firewire/core-transaction.c
@@ -921,23 +921,15 @@ static void handle_registers(struct fw_c
void *payload, size_t length, void *callback_data)
{
int reg = offset & ~CSR_REGISTER_BASE;
- unsigned long long bus_time;
__be32 *data = payload;
int rcode = RCODE_COMPLETE;
switch (reg) {
case CSR_CYCLE_TIME:
- case CSR_BUS_TIME:
- if (!TCODE_IS_READ_REQUEST(tcode) || length != 4) {
- rcode = RCODE_TYPE_ERROR;
- break;
- }
-
- bus_time = card->driver->get_bus_time(card);
- if (reg == CSR_CYCLE_TIME)
- *data = cpu_to_be32(bus_time);
+ if (TCODE_IS_READ_REQUEST(tcode) && length == 4)
+ *data = cpu_to_be32(card->driver->get_cycle_time(card));
else
- *data = cpu_to_be32(bus_time >> 25);
+ rcode = RCODE_TYPE_ERROR;
break;
case CSR_BROADCAST_CHANNEL:
@@ -968,6 +960,9 @@ static void handle_registers(struct fw_c
case CSR_BUSY_TIMEOUT:
/* FIXME: Implement this. */
+ case CSR_BUS_TIME:
+ /* Useless without initialization by the bus manager. */
+
default:
rcode = RCODE_ADDRESS_ERROR;
break;
Index: linux-2.6.33-rc8/drivers/firewire/core.h
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/core.h
+++ linux-2.6.33-rc8/drivers/firewire/core.h
@@ -70,7 +70,7 @@ struct fw_card_driver {
int (*enable_phys_dma)(struct fw_card *card,
int node_id, int generation);
- u64 (*get_bus_time)(struct fw_card *card);
+ u32 (*get_cycle_time)(struct fw_card *card);
struct fw_iso_context *
(*allocate_iso_context)(struct fw_card *card,
Index: linux-2.6.33-rc8/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.33-rc8.orig/drivers/firewire/ohci.c
+++ linux-2.6.33-rc8/drivers/firewire/ohci.c
@@ -38,7 +38,6 @@
#include <linux/spinlock.h>
#include <linux/string.h>
-#include <asm/atomic.h>
#include <asm/byteorder.h>
#include <asm/page.h>
#include <asm/system.h>
@@ -187,7 +186,6 @@ struct fw_ohci {
int node_id;
int generation;
int request_generation; /* for timestamping incoming requests */
- atomic_t bus_seconds;
bool use_dualbuffer;
bool old_uninorth;
@@ -276,7 +274,7 @@ static void log_irqs(u32 evt)
!(evt & OHCI1394_busReset))
return;
- fw_notify("IRQ %08x%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n", evt,
+ fw_notify("IRQ %08x%s%s%s%s%s%s%s%s%s%s%s%s%s\n", evt,
evt & OHCI1394_selfIDComplete ? " selfID" : "",
evt & OHCI1394_RQPkt ? " AR_req" : "",
evt & OHCI1394_RSPkt ? " AR_resp" : "",
@@ -286,7 +284,6 @@ static void log_irqs(u32 evt)
evt & OHCI1394_isochTx ? " IT" : "",
evt & OHCI1394_postedWriteErr ? " postedWriteErr" : "",
evt & OHCI1394_cycleTooLong ? " cycleTooLong" : "",
- evt & OHCI1394_cycle64Seconds ? " cycle64Seconds" : "",
evt & OHCI1394_cycleInconsistent ? " cycleInconsistent" : "",
evt & OHCI1394_regAccessFail ? " regAccessFail" : "",
evt & OHCI1394_busReset ? " busReset" : "",
@@ -294,8 +291,7 @@ static void log_irqs(u32 evt)
OHCI1394_RSPkt | OHCI1394_reqTxComplete |
OHCI1394_respTxComplete | OHCI1394_isochRx |
OHCI1394_isochTx | OHCI1394_postedWriteErr |
- OHCI1394_cycleTooLong | OHCI1394_cycle64Seconds |
- OHCI1394_cycleInconsistent |
+ OHCI1394_cycleTooLong | OHCI1394_cycleInconsistent |
OHCI1394_regAccessFail | OHCI1394_busReset)
? " ?" : "");
}
@@ -1385,7 +1381,7 @@ static void bus_reset_tasklet(unsigned l
static irqreturn_t irq_handler(int irq, void *data)
{
struct fw_ohci *ohci = data;
- u32 event, iso_event, cycle_time;
+ u32 event, iso_event;
int i;
event = reg_read(ohci, OHCI1394_IntEventClear);
@@ -1455,12 +1451,6 @@ static irqreturn_t irq_handler(int irq,
fw_notify("isochronous cycle inconsistent\n");
}
- if (event & OHCI1394_cycle64Seconds) {
- cycle_time = reg_read(ohci, OHCI1394_IsochronousCycleTimer);
- if ((cycle_time & 0x80000000) == 0)
- atomic_inc(&ohci->bus_seconds);
- }
-
return IRQ_HANDLED;
}
@@ -1554,8 +1544,7 @@ static int ohci_enable(struct fw_card *c
OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
OHCI1394_isochRx | OHCI1394_isochTx |
OHCI1394_postedWriteErr | OHCI1394_cycleTooLong |
- OHCI1394_cycleInconsistent |
- OHCI1394_cycle64Seconds | OHCI1394_regAccessFail |
+ OHCI1394_cycleInconsistent | OHCI1394_regAccessFail |
OHCI1394_masterIntEnable);
if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset);
@@ -1821,7 +1810,7 @@ static u32 cycle_timer_ticks(u32 cycle_t
* error. (A PCI read should take at least 20 ticks of the 24.576 MHz timer to
* execute, so we have enough precision to compute the ratio of the differences.)
*/
-static u64 ohci_get_bus_time(struct fw_card *card)
+static u32 ohci_get_cycle_time(struct fw_card *card)
{
struct fw_ohci *ohci = fw_ohci(card);
u32 c0, c1, c2;
@@ -1849,7 +1838,7 @@ static u64 ohci_get_bus_time(struct fw_c
&& i++ < 20);
}
- return ((u64)atomic_read(&ohci->bus_seconds) << 32) | c2;
+ return c2;
}
static void copy_iso_headers(struct iso_context *ctx, void *p)
@@ -2429,7 +2418,7 @@ static const struct fw_card_driver ohci_
.send_response = ohci_send_response,
.cancel_packet = ohci_cancel_packet,
.enable_phys_dma = ohci_enable_phys_dma,
- .get_bus_time = ohci_get_bus_time,
+ .get_cycle_time = ohci_get_cycle_time,
.allocate_iso_context = ohci_allocate_iso_context,
.free_iso_context = ohci_free_iso_context,
--
Stefan Richter
-=====-==-=- --=- -===-
http://arcgraph.de/sr/
On 14 Feb, Stefan Richter wrote:
> --- linux-2.6.33-rc8.orig/drivers/firewire/core-cdev.c
> +++ linux-2.6.33-rc8/drivers/firewire/core-cdev.c
[...]
> @@ -32,8 +33,6 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> -#include <linux/preempt.h>
> -#include <linux/sched.h>
> #include <linux/spinlock.h>
> #include <linux/string.h>
> #include <linux/time.h>
[...]
This doesn't compile on some configurations. core-cdev.c requires (and
includes) linux/wait.h which in turn requires (but does not include)
linux/sched.h.
However, adding #include <linux/sched.h> in wait.h isn't going to be
possible very easily; there are a whole lot of missing definitions in
other headers that are pulled in by sched.h then.
Hence I will simply readd sched.h to firewire/core-cdev.c.
--
Stefan Richter
-=====-==-=- --=- =-=--
http://arcgraph.de/sr/