fw-ohci breaks suspend-to-RAM

fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 332 bytes --]

Hi,

the fw-ohci driver breaks suspend to RAM on my Thinkpad R60, Intel Core 2 Duo, 
kernel 2.6.22-rc1 (x86_64, SMP). It does suspend but hangs on resume if the 
fw-ohci module is loaded. (See also the original report [1]).

Best regards,

Marcus

[1] http://permalink.gmane.org/gmane.linux.power-management.general/5819

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: fw-ohci breaks suspend-to-RAM

[Andrew Morton]

On Fri, 18 May 2007 12:07:40 +0200 Marcus Better <marcus@better.se> wrote:

> Hi,
> 
> the fw-ohci driver breaks suspend to RAM on my Thinkpad R60, Intel Core 2 Duo, 
> kernel 2.6.22-rc1 (x86_64, SMP). It does suspend but hangs on resume if the 
> fw-ohci module is loaded. (See also the original report [1]).
> 
> Best regards,
> 
> Marcus
> 
> [1] http://permalink.gmane.org/gmane.linux.power-management.general/5819
> 

(added linux1394-devel to cc)

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Andrew Morton wrote:
> On Fri, 18 May 2007 12:07:40 +0200 Marcus Better <marcus@better.se> wrote:
>> the fw-ohci driver breaks suspend to RAM on my Thinkpad R60, Intel Core 2 Duo, 
>> kernel 2.6.22-rc1 (x86_64, SMP). It does suspend but hangs on resume if the 
>> fw-ohci module is loaded. (See also the original report [1]).
...
>> [1] http://permalink.gmane.org/gmane.linux.power-management.general/5819

Marcus, thanks for the report.  I will try to reproduce and fix it
unless Kristian is faster.
-- 
Stefan Richter
-=====-=-=== -=-= =--=-
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

>> On Fri, 18 May 2007 12:07:40 +0200 Marcus Better <marcus@better.se> wrote:
>>> the fw-ohci driver breaks suspend to RAM on my Thinkpad R60, Intel Core 2 Duo, 
>>> kernel 2.6.22-rc1 (x86_64, SMP). It does suspend but hangs on resume if the 
>>> fw-ohci module is loaded.

Could you test the following patch?  I don't have a machine to test it
myself at the moment but may have one tonight or tomorrow.


From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: firewire: simple support for suspend/resume

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |   37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1716,8 +1716,11 @@ static int software_reset(struct fw_ohci
 	return -EBUSY;
 }
 
-static int __devinit
-pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
+static int
+#ifndef CONFIG_PM
+__devinit
+#endif
+pci_probe_or_resume(struct pci_dev *dev)
 {
 	struct fw_ohci *ohci;
 	u32 bus_options, max_receive, link_speed;
@@ -1879,6 +1882,12 @@ pci_probe(struct pci_dev *dev, const str
 	return err;
 }
 
+static int __devinit
+pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
+{
+	return pci_probe_or_resume(dev);
+}
+
 static void pci_remove(struct pci_dev *dev)
 {
 	struct fw_ohci *ohci;
@@ -1907,6 +1916,26 @@ static void pci_remove(struct pci_dev *d
 	fw_notify("Removed fw-ohci device.\n");
 }
 
+#ifdef CONFIG_PM
+/*
+ * FIXME: Implement it as transparent to upper layers as possible.  To those
+ * drivers without special requirements, a suspend--resume cycle should merely
+ * look like a prolonged bus reset phase.  For example, fw-sbp2 should just
+ * re-login after resume.
+ */
+static int pci_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_remove(dev);
+	return 0;
+}
+
+static int pci_resume(struct pci_dev *dev)
+{
+	pci_probe_or_resume(dev);
+	return 0;
+}
+#endif /* CONFIG_PM */
+
 static struct pci_device_id pci_table[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
 	{ }
@@ -1919,6 +1948,10 @@ static struct pci_driver fw_ohci_pci_dri
 	.id_table	= pci_table,
 	.probe		= pci_probe,
 	.remove		= pci_remove,
+#ifdef CONFIG_PM
+	.suspend	= pci_suspend,
+	.resume		= pci_resume,
+#endif
 };
 
 MODULE_AUTHOR("Kristian Hoegsberg <krh@bitplanet.net>");



-- 
Stefan Richter
-=====-=-=== -=-= =-=--
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

I wrote:
> Could you test the following patch?  I don't have a machine to test it
> myself at the moment but may have one tonight or tomorrow.

Sorry.  I tried with a new ACPI PC and and old APM notebook.  The former
does not resume after s2ram (screen remains blank after resume), and the
latter has a serious bug in APM resume under 2.6.22-rc2 (without
FireWire drivers loaded but with some other drivers present; it resumes
but anything I do quickly locks up, even "dmesg").

I will try to get 2.6.21 or .20 plus backports of the new FireWire
drivers set up on the APM notebook during next week.  But I'd appreciate
if you could test the patch from my previous post.
-- 
Stefan Richter
-=====-=-=== -=-= =-=--
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 151 bytes --]

Stefan Richter wrote:
> But I'd appreciate if you could test the patch from my previous post.

I will test it tomorrow morning (CET).

Thanks,

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 137 bytes --]

Stefan Richter wrote:
> Could you test the following patch?

Now it hangs on the second suspend to RAM (at suspend, not resume).

Marcus

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Marcus Better wrote:
> Now it hangs on the second suspend to RAM (at suspend, not resume).

Unless Kristian implements something better before release, we might go
with the following patch.  Does it abort s2ram as I intend?

(It does not abort APM suspend, but on the APM notebook I tested with
2.6.20 and the new firewire drivers, I could unload and reload fw-ohci
after APM resume without problem.  I still have no machine which
supports s2ram, and the APM machine is too slow to do driver development
on it.  I might try to install suspend2 for hibernate-to-swapfile on a
reasonable machine, but not earlier than in two weeks.  Therefore don't
expect anything better than the below from me anytime soon.)

 
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: firewire: suspend/resume is unimplemented

2.6.22-rc1 reportedly hangs in resume if fw-ohci is loaded.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1908,6 +1908,19 @@ static void pci_remove(struct pci_dev *d
 	fw_notify("Removed fw-ohci device.\n");
 }
 
+#ifdef CONFIG_PM
+static int pci_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	fw_notify("ERROR: Unload this driver before hibernate or suspend\n");
+	return -ENOSYS;
+}
+
+static int pci_resume(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+#endif /* CONFIG_PM */
+
 static struct pci_device_id pci_table[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
 	{ }
@@ -1920,6 +1933,10 @@ static struct pci_driver fw_ohci_pci_dri
 	.id_table	= pci_table,
 	.probe		= pci_probe,
 	.remove		= pci_remove,
+#ifdef CONFIG_PM
+	.suspend	= pci_suspend,
+	.resume		= pci_resume,
+#endif
 };
 
 MODULE_AUTHOR("Kristian Hoegsberg <krh@bitplanet.net>");


-- 
Stefan Richter
-=====-=-=== -=-= ==-=-
http://arcgraph.de/sr/

Re: Re: fw-ohci breaks suspend-to-RAM

[Rafael J. Wysocki]

On Saturday, 26 May 2007 14:14, Stefan Richter wrote:
> Marcus Better wrote:
> > Now it hangs on the second suspend to RAM (at suspend, not resume).
> 
> Unless Kristian implements something better before release, we might go
> with the following patch.  Does it abort s2ram as I intend?
> 
> (It does not abort APM suspend, but on the APM notebook I tested with
> 2.6.20 and the new firewire drivers, I could unload and reload fw-ohci
> after APM resume without problem.  I still have no machine which
> supports s2ram, and the APM machine is too slow to do driver development
> on it.  I might try to install suspend2 for hibernate-to-swapfile on a
> reasonable machine, but not earlier than in two weeks.  Therefore don't
> expect anything better than the below from me anytime soon.)

Actually you can try swsusp too (the built-in stuff).  As far as the handling
of devices is concerned it's pretty much equivalent to suspend2 and you don't
need to patch the kernel in order to use it.

Greetings,
Rafael

Re: Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Rafael J. Wysocki wrote:
> On Saturday, 26 May 2007 14:14, Stefan Richter wrote:
>> I might try to install suspend2 for hibernate-to-swapfile

> Actually you can try swsusp too (the built-in stuff).

I don't have a swap partition.
-- 
Stefan Richter
-=====-=-=== -=-= ==-=-
http://arcgraph.de/sr/

Re: Re: fw-ohci breaks suspend-to-RAM

[Rafael J. Wysocki]

On Saturday, 26 May 2007 23:24, Stefan Richter wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, 26 May 2007 14:14, Stefan Richter wrote:
> >> I might try to install suspend2 for hibernate-to-swapfile
> 
> > Actually you can try swsusp too (the built-in stuff).
> 
> I don't have a swap partition.

Please see Documentation/power/swsusp-and-swap-files.txt

In the suspend package containing s2ram there is the program swap-offset
that you can use to obtain the resume_offset value for your swap file
(NOTE: this doesn't work with swap files on LVM).

Greetings,
Rafael

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Marcus, please test:

From: =?utf-8?q?Kristian_H=C3=B8gsberg?= <krh@redhat.com>
Subject: [PATCH] Implement suspend/resume PCI driver hooks.
Date: Wed, 30 May 2007 19:06:35 -0400

It's a low-impact design, that just makes suspend/resume look like
a bus reset to the upper level drivers, but it should be sufficient.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---

 drivers/firewire/fw-card.c |    5 -
 drivers/firewire/fw-ohci.c |  187 +++++++++++++++++++++++++++++---------------
 2 files changed, 123 insertions(+), 69 deletions(-)

diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c
index 636151a..9eb1eda 100644
--- a/drivers/firewire/fw-card.c
+++ b/drivers/firewire/fw-card.c
@@ -407,11 +407,6 @@ fw_card_add(struct fw_card *card,
 	card->link_speed = link_speed;
 	card->guid = guid;
 
-	/* Activate link_on bit and contender bit in our self ID packets.*/
-	if (card->driver->update_phy_reg(card, 4, 0,
-					 PHY_LINK_ACTIVE | PHY_CONTENDER) < 0)
-		return -EIO;
-
 	/*
 	 * The subsystem grabs a reference when the card is added and
 	 * drops it when the driver calls fw_core_remove_card.
diff --git a/drivers/firewire/fw-ohci.c b/drivers/firewire/fw-ohci.c
index 2e4cfa5..7db2cee 100644
--- a/drivers/firewire/fw-ohci.c
+++ b/drivers/firewire/fw-ohci.c
@@ -417,12 +417,21 @@ ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci, u32 regs)
 	ctx->current_buffer = ab.next;
 	ctx->pointer = ctx->current_buffer->data;
 
-	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs),
-		  le32_to_cpu(ab.descriptor.branch_address));
+	return 0;
+}
+
+static void ar_context_run(struct ar_context *ctx)
+{
+	struct ar_buffer *ab = ctx->current_buffer;
+	dma_addr_t ab_bus;
+	size_t offset;
+	
+	offset = offsetof(struct ar_buffer, data);
+	ab_bus = ab->descriptor.data_address - offset;
+
+	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs), ab_bus | 1);
 	reg_write(ctx->ohci, CONTROL_SET(ctx->regs), CONTEXT_RUN);
 	flush_writes(ctx->ohci);
-
-	return 0;
 }
 
 static void context_tasklet(unsigned long data)
@@ -1039,11 +1048,78 @@ static irqreturn_t irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int software_reset(struct fw_ohci *ohci)
+{
+	int i;
+
+	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
+
+	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
+		if ((reg_read(ohci, OHCI1394_HCControlSet) &
+		     OHCI1394_HCControl_softReset) == 0)
+			return 0;
+		msleep(1);
+	}
+
+	return -EBUSY;
+}
+
 static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length)
 {
 	struct fw_ohci *ohci = fw_ohci(card);
 	struct pci_dev *dev = to_pci_dev(card->device);
 
+	if (software_reset(ohci)) {
+		fw_error("Failed to reset ohci card.\n");
+		return -EBUSY;
+	}
+
+	/*
+	 * Now enable LPS, which we need in order to start accessing
+	 * most of the registers.  In fact, on some cards (ALI M5251),
+	 * accessing registers in the SClk domain without LPS enabled
+	 * will lock up the machine.  Wait 50msec to make sure we have
+	 * full link enabled.
+	 */
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_LPS |
+		  OHCI1394_HCControl_postedWriteEnable);
+	flush_writes(ohci);
+	msleep(50);
+
+	reg_write(ohci, OHCI1394_HCControlClear,
+		  OHCI1394_HCControl_noByteSwapData);
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  OHCI1394_LinkControl_rcvSelfID |
+		  OHCI1394_LinkControl_cycleTimerEnable |
+		  OHCI1394_LinkControl_cycleMaster);
+
+	reg_write(ohci, OHCI1394_ATRetries,
+		  OHCI1394_MAX_AT_REQ_RETRIES |
+		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
+		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
+
+	ar_context_run(&ohci->ar_request_ctx);
+	ar_context_run(&ohci->ar_response_ctx);
+
+	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
+	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
+	reg_write(ohci, OHCI1394_IntEventClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskSet,
+		  OHCI1394_selfIDComplete |
+		  OHCI1394_RQPkt | OHCI1394_RSPkt |
+		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
+		  OHCI1394_isochRx | OHCI1394_isochTx |
+		  OHCI1394_masterIntEnable |
+		  OHCI1394_cycle64Seconds);
+
+	/* Activate link_on bit and contender bit in our self ID packets.*/
+	if (ohci_update_phy_reg(card, 4, 0,
+				PHY_LINK_ACTIVE | PHY_CONTENDER) < 0)
+		return -EIO;
+
 	/*
 	 * When the link is not yet enabled, the atomic config rom
 	 * update mechanism described below in ohci_set_config_rom()
@@ -1701,22 +1777,6 @@ static const struct fw_card_driver ohci_driver = {
 	.stop_iso		= ohci_stop_iso,
 };
 
-static int software_reset(struct fw_ohci *ohci)
-{
-	int i;
-
-	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
-
-	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
-		if ((reg_read(ohci, OHCI1394_HCControlSet) &
-		     OHCI1394_HCControl_softReset) == 0)
-			return 0;
-		msleep(1);
-	}
-
-	return -EBUSY;
-}
-
 static int __devinit
 pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 {
@@ -1762,33 +1822,6 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 		goto fail_iomem;
 	}
 
-	if (software_reset(ohci)) {
-		fw_error("Failed to reset ohci card.\n");
-		err = -EBUSY;
-		goto fail_registers;
-	}
-
-	/*
-	 * Now enable LPS, which we need in order to start accessing
-	 * most of the registers.  In fact, on some cards (ALI M5251),
-	 * accessing registers in the SClk domain without LPS enabled
-	 * will lock up the machine.  Wait 50msec to make sure we have
-	 * full link enabled.
-	 */
-	reg_write(ohci, OHCI1394_HCControlSet,
-		  OHCI1394_HCControl_LPS |
-		  OHCI1394_HCControl_postedWriteEnable);
-	flush_writes(ohci);
-	msleep(50);
-
-	reg_write(ohci, OHCI1394_HCControlClear,
-		  OHCI1394_HCControl_noByteSwapData);
-
-	reg_write(ohci, OHCI1394_LinkControlSet,
-		  OHCI1394_LinkControl_rcvSelfID |
-		  OHCI1394_LinkControl_cycleTimerEnable |
-		  OHCI1394_LinkControl_cycleMaster);
-
 	ar_context_init(&ohci->ar_request_ctx, ohci,
 			OHCI1394_AsReqRcvContextControlSet);
 
@@ -1801,11 +1834,6 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	context_init(&ohci->at_response_ctx, ohci, AT_BUFFER_SIZE,
 		     OHCI1394_AsRspTrContextControlSet, handle_at_packet);
 
-	reg_write(ohci, OHCI1394_ATRetries,
-		  OHCI1394_MAX_AT_REQ_RETRIES |
-		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
-		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
-
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0);
 	ohci->it_context_mask = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet);
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0);
@@ -1835,18 +1863,6 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 		goto fail_registers;
 	}
 
-	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
-	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
-	reg_write(ohci, OHCI1394_IntEventClear, ~0);
-	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
-	reg_write(ohci, OHCI1394_IntMaskSet,
-		  OHCI1394_selfIDComplete |
-		  OHCI1394_RQPkt | OHCI1394_RSPkt |
-		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
-		  OHCI1394_isochRx | OHCI1394_isochTx |
-		  OHCI1394_masterIntEnable |
-		  OHCI1394_cycle64Seconds);
-
 	bus_options = reg_read(ohci, OHCI1394_BusOptions);
 	max_receive = (bus_options >> 12) & 0xf;
 	link_speed = bus_options & 0x7;
@@ -1908,6 +1924,45 @@ static void pci_remove(struct pci_dev *dev)
 	fw_notify("Removed fw-ohci device.\n");
 }
 
+#ifdef CONFIG_PM
+static int pci_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	struct fw_ohci *ohci = pci_get_drvdata(pdev);
+	int err;
+
+	software_reset(ohci);
+	free_irq(pdev->irq, ohci);
+	err = pci_save_state(pdev);
+	if (err) {
+		fw_error("pci_save_state failed with %d", err);
+		return err;
+	}
+	err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	if (err) {
+		fw_error("pci_set_power_state failed with %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int pci_resume(struct pci_dev *pdev)
+{
+	struct fw_ohci *ohci = pci_get_drvdata(pdev);
+	int err;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	err = pci_enable_device(pdev);
+	if (err) {
+		fw_error("pci_enable_device failed with %d", err);
+		return err;
+	}
+
+	return ohci_enable(&ohci->card, ohci->config_rom, CONFIG_ROM_SIZE);
+}
+#endif
+
 static struct pci_device_id pci_table[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
 	{ }
@@ -1920,6 +1975,10 @@ static struct pci_driver fw_ohci_pci_driver = {
 	.id_table	= pci_table,
 	.probe		= pci_probe,
 	.remove		= pci_remove,
+#ifdef CONFIG_PM
+	.resume		= pci_resume,
+	.suspend	= pci_suspend,
+#endif
 };
 
 MODULE_AUTHOR("Kristian Hoegsberg <krh@bitplanet.net>");



-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 415 bytes --]

Stefan Richter wrote:
> Marcus, please test:

> From: =?utf-8?q?Kristian_H=C3=B8gsberg?= <krh@redhat.com>

I have trouble applying this patch, perhaps due to the encoding. Can you 
resend it without quoted-printable? (Or am I doing something wrong?)

$ patch -p1 --dry-run < /home/marcus/fw.diff
patching file drivers/firewire/fw-card.c
patch: **** malformed patch at line 11: --- 408,407 ----

Marcus

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

On 31 May, Marcus Better wrote:
> I have trouble applying this patch, perhaps due to the encoding. Can you 
> resend it without quoted-printable? (Or am I doing something wrong?)
> 
> $ patch -p1 --dry-run < /home/marcus/fw.diff
> patching file drivers/firewire/fw-card.c
> patch: **** malformed patch at line 11: --- 408,407 ----
> 
> Marcus


If you have linux-2.6.22-rc1...-rc3 (not the latest -git), then you need
the following patch first in order to apply the suspend/resume patch
cleanly.  (I will follow up with a ASCIIfied version of the
suspend/resume patch.)


From: Kristian Hoegsberg <krh@redhat.com>
Subject: firewire: Add missing byteswapping for receive DMA programs.
Date: Tue, 22 May 2007 18:55:48 -0400

Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc3/drivers/firewire/fw-ohci.c
===================================================================
--- linux-2.6.22-rc3.orig/drivers/firewire/fw-ohci.c
+++ linux-2.6.22-rc3/drivers/firewire/fw-ohci.c
@@ -268,7 +268,7 @@ static int ar_context_add_page(struct ar
 
 	dma_sync_single_for_device(dev, ab_bus, PAGE_SIZE, DMA_BIDIRECTIONAL);
 
-	ctx->last_buffer->descriptor.branch_address = ab_bus | 1;
+	ctx->last_buffer->descriptor.branch_address = cpu_to_le32(ab_bus | 1);
 	ctx->last_buffer->next = ab;
 	ctx->last_buffer = ab;
 
@@ -417,7 +417,8 @@ ar_context_init(struct ar_context *ctx, 
 	ctx->current_buffer = ab.next;
 	ctx->pointer = ctx->current_buffer->data;
 
-	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs), ab.descriptor.branch_address);
+	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs),
+		  le32_to_cpu(ab.descriptor.branch_address));
 	reg_write(ctx->ohci, CONTROL_SET(ctx->regs), CONTEXT_RUN);
 	flush_writes(ctx->ohci);
 




-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

> (I will follow up with a ASCIIfied version of the
> suspend/resume patch.)

From: Kristian Hoegsberg <krh@redhat.com>
Subject: [PATCH] Implement suspend/resume PCI driver hooks.
Date: Wed, 30 May 2007 19:06:35 -0400

It's a low-impact design, that just makes suspend/resume look like
a bus reset to the upper level drivers, but it should be sufficient.

Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
---

 drivers/firewire/fw-card.c |    5 
 drivers/firewire/fw-ohci.c |  187 ++++++++++++++++++++++++-------------
 2 files changed, 123 insertions(+), 69 deletions(-)

Index: linux-2.6.22-rc3/drivers/firewire/fw-card.c
===================================================================
--- linux-2.6.22-rc3.orig/drivers/firewire/fw-card.c
+++ linux-2.6.22-rc3/drivers/firewire/fw-card.c
@@ -407,11 +407,6 @@ fw_card_add(struct fw_card *card,
 	card->link_speed = link_speed;
 	card->guid = guid;
 
-	/* Activate link_on bit and contender bit in our self ID packets.*/
-	if (card->driver->update_phy_reg(card, 4, 0,
-					 PHY_LINK_ACTIVE | PHY_CONTENDER) < 0)
-		return -EIO;
-
 	/*
 	 * The subsystem grabs a reference when the card is added and
 	 * drops it when the driver calls fw_core_remove_card.
Index: linux-2.6.22-rc3/drivers/firewire/fw-ohci.c
===================================================================
--- linux-2.6.22-rc3.orig/drivers/firewire/fw-ohci.c
+++ linux-2.6.22-rc3/drivers/firewire/fw-ohci.c
@@ -417,12 +417,21 @@ ar_context_init(struct ar_context *ctx, 
 	ctx->current_buffer = ab.next;
 	ctx->pointer = ctx->current_buffer->data;
 
-	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs),
-		  le32_to_cpu(ab.descriptor.branch_address));
+	return 0;
+}
+
+static void ar_context_run(struct ar_context *ctx)
+{
+	struct ar_buffer *ab = ctx->current_buffer;
+	dma_addr_t ab_bus;
+	size_t offset;
+
+	offset = offsetof(struct ar_buffer, data);
+	ab_bus = ab->descriptor.data_address - offset;
+
+	reg_write(ctx->ohci, COMMAND_PTR(ctx->regs), ab_bus | 1);
 	reg_write(ctx->ohci, CONTROL_SET(ctx->regs), CONTEXT_RUN);
 	flush_writes(ctx->ohci);
-
-	return 0;
 }
 
 static void context_tasklet(unsigned long data)
@@ -1039,11 +1048,78 @@ static irqreturn_t irq_handler(int irq, 
 	return IRQ_HANDLED;
 }
 
+static int software_reset(struct fw_ohci *ohci)
+{
+	int i;
+
+	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
+
+	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
+		if ((reg_read(ohci, OHCI1394_HCControlSet) &
+		     OHCI1394_HCControl_softReset) == 0)
+			return 0;
+		msleep(1);
+	}
+
+	return -EBUSY;
+}
+
 static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length)
 {
 	struct fw_ohci *ohci = fw_ohci(card);
 	struct pci_dev *dev = to_pci_dev(card->device);
 
+	if (software_reset(ohci)) {
+		fw_error("Failed to reset ohci card.\n");
+		return -EBUSY;
+	}
+
+	/*
+	 * Now enable LPS, which we need in order to start accessing
+	 * most of the registers.  In fact, on some cards (ALI M5251),
+	 * accessing registers in the SClk domain without LPS enabled
+	 * will lock up the machine.  Wait 50msec to make sure we have
+	 * full link enabled.
+	 */
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_LPS |
+		  OHCI1394_HCControl_postedWriteEnable);
+	flush_writes(ohci);
+	msleep(50);
+
+	reg_write(ohci, OHCI1394_HCControlClear,
+		  OHCI1394_HCControl_noByteSwapData);
+
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  OHCI1394_LinkControl_rcvSelfID |
+		  OHCI1394_LinkControl_cycleTimerEnable |
+		  OHCI1394_LinkControl_cycleMaster);
+
+	reg_write(ohci, OHCI1394_ATRetries,
+		  OHCI1394_MAX_AT_REQ_RETRIES |
+		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
+		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
+
+	ar_context_run(&ohci->ar_request_ctx);
+	ar_context_run(&ohci->ar_response_ctx);
+
+	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
+	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
+	reg_write(ohci, OHCI1394_IntEventClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskSet,
+		  OHCI1394_selfIDComplete |
+		  OHCI1394_RQPkt | OHCI1394_RSPkt |
+		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
+		  OHCI1394_isochRx | OHCI1394_isochTx |
+		  OHCI1394_masterIntEnable |
+		  OHCI1394_cycle64Seconds);
+
+	/* Activate link_on bit and contender bit in our self ID packets.*/
+	if (ohci_update_phy_reg(card, 4, 0,
+				PHY_LINK_ACTIVE | PHY_CONTENDER) < 0)
+		return -EIO;
+
 	/*
 	 * When the link is not yet enabled, the atomic config rom
 	 * update mechanism described below in ohci_set_config_rom()
@@ -1701,22 +1777,6 @@ static const struct fw_card_driver ohci_
 	.stop_iso		= ohci_stop_iso,
 };
 
-static int software_reset(struct fw_ohci *ohci)
-{
-	int i;
-
-	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
-
-	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
-		if ((reg_read(ohci, OHCI1394_HCControlSet) &
-		     OHCI1394_HCControl_softReset) == 0)
-			return 0;
-		msleep(1);
-	}
-
-	return -EBUSY;
-}
-
 static int __devinit
 pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 {
@@ -1762,33 +1822,6 @@ pci_probe(struct pci_dev *dev, const str
 		goto fail_iomem;
 	}
 
-	if (software_reset(ohci)) {
-		fw_error("Failed to reset ohci card.\n");
-		err = -EBUSY;
-		goto fail_registers;
-	}
-
-	/*
-	 * Now enable LPS, which we need in order to start accessing
-	 * most of the registers.  In fact, on some cards (ALI M5251),
-	 * accessing registers in the SClk domain without LPS enabled
-	 * will lock up the machine.  Wait 50msec to make sure we have
-	 * full link enabled.
-	 */
-	reg_write(ohci, OHCI1394_HCControlSet,
-		  OHCI1394_HCControl_LPS |
-		  OHCI1394_HCControl_postedWriteEnable);
-	flush_writes(ohci);
-	msleep(50);
-
-	reg_write(ohci, OHCI1394_HCControlClear,
-		  OHCI1394_HCControl_noByteSwapData);
-
-	reg_write(ohci, OHCI1394_LinkControlSet,
-		  OHCI1394_LinkControl_rcvSelfID |
-		  OHCI1394_LinkControl_cycleTimerEnable |
-		  OHCI1394_LinkControl_cycleMaster);
-
 	ar_context_init(&ohci->ar_request_ctx, ohci,
 			OHCI1394_AsReqRcvContextControlSet);
 
@@ -1801,11 +1834,6 @@ pci_probe(struct pci_dev *dev, const str
 	context_init(&ohci->at_response_ctx, ohci, AT_BUFFER_SIZE,
 		     OHCI1394_AsRspTrContextControlSet, handle_at_packet);
 
-	reg_write(ohci, OHCI1394_ATRetries,
-		  OHCI1394_MAX_AT_REQ_RETRIES |
-		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
-		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
-
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0);
 	ohci->it_context_mask = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet);
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0);
@@ -1835,18 +1863,6 @@ pci_probe(struct pci_dev *dev, const str
 		goto fail_registers;
 	}
 
-	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
-	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
-	reg_write(ohci, OHCI1394_IntEventClear, ~0);
-	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
-	reg_write(ohci, OHCI1394_IntMaskSet,
-		  OHCI1394_selfIDComplete |
-		  OHCI1394_RQPkt | OHCI1394_RSPkt |
-		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
-		  OHCI1394_isochRx | OHCI1394_isochTx |
-		  OHCI1394_masterIntEnable |
-		  OHCI1394_cycle64Seconds);
-
 	bus_options = reg_read(ohci, OHCI1394_BusOptions);
 	max_receive = (bus_options >> 12) & 0xf;
 	link_speed = bus_options & 0x7;
@@ -1908,6 +1924,45 @@ static void pci_remove(struct pci_dev *d
 	fw_notify("Removed fw-ohci device.\n");
 }
 
+#ifdef CONFIG_PM
+static int pci_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	struct fw_ohci *ohci = pci_get_drvdata(pdev);
+	int err;
+
+	software_reset(ohci);
+	free_irq(pdev->irq, ohci);
+	err = pci_save_state(pdev);
+	if (err) {
+		fw_error("pci_save_state failed with %d", err);
+		return err;
+	}
+	err = pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	if (err) {
+		fw_error("pci_set_power_state failed with %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int pci_resume(struct pci_dev *pdev)
+{
+	struct fw_ohci *ohci = pci_get_drvdata(pdev);
+	int err;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	err = pci_enable_device(pdev);
+	if (err) {
+		fw_error("pci_enable_device failed with %d", err);
+		return err;
+	}
+
+	return ohci_enable(&ohci->card, ohci->config_rom, CONFIG_ROM_SIZE);
+}
+#endif
+
 static struct pci_device_id pci_table[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_FIREWIRE_OHCI, ~0) },
 	{ }
@@ -1920,6 +1975,10 @@ static struct pci_driver fw_ohci_pci_dri
 	.id_table	= pci_table,
 	.probe		= pci_probe,
 	.remove		= pci_remove,
+#ifdef CONFIG_PM
+	.resume		= pci_resume,
+	.suspend	= pci_suspend,
+#endif
 };
 
 MODULE_AUTHOR("Kristian Hoegsberg <krh@bitplanet.net>");


-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 169 bytes --]

Stefan Richter wrote

> From: Kristian Hoegsberg <krh@redhat.com>
> Subject: [PATCH] Implement suspend/resume PCI driver hooks.

Yes, it seems to work.

Thanks,

Marcus

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Marcus, thanks for the fast feedback.
-- 
Stefan Richter
-=====-=-=== -=-= =====
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Kristian Høgsberg]

On 5/31/07, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Marcus, thanks for the fast feedback.

I've posted an updated version of the patch here that fixes a
byteswapping problem, and should make the stack actually work after
resuming:

http://marc.info/?l=linux1394-devel&m=118108604226326&w=2

cheers,
Kristian

Re: fw-ohci breaks suspend-to-RAM

[Stefan Richter]

Kristian Høgsberg wrote:
> I've posted an updated version of the patch here
                 ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
You mean "addition to the previous" patch. :-)

> that fixes a byteswapping problem, and should make the stack
> actually work after resuming:
> 
> http://marc.info/?l=linux1394-devel&m=118108604226326&w=2

That is, protocols which do not involve showing the local ROM to remote
nodes are unaffected?  In that case I would prefer to defer this new
patch until the next merge window if you don't mind.  Nevertheless, if
Marcus could take the time to check whether his machine keeps happily
resuming with the new patch applied, that would be good.  I will test it
with APM suspend and SBP-2 circa tomorrow.
-- 
Stefan Richter
-=====-=-=== -==- --==-
http://arcgraph.de/sr/

Re: fw-ohci breaks suspend-to-RAM

[Kristian Høgsberg]

On 6/6/07, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Kristian Høgsberg wrote:
> > I've posted an updated version of the patch here
>                  ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
> You mean "addition to the previous" patch. :-)

Yes, sorry about that.

> > that fixes a byteswapping problem, and should make the stack
> > actually work after resuming:
> >
> > http://marc.info/?l=linux1394-devel&m=118108604226326&w=2
>
> That is, protocols which do not involve showing the local ROM to remote
> nodes are unaffected?  In that case I would prefer to defer this new
> patch until the next merge window if you don't mind.  Nevertheless, if
> Marcus could take the time to check whether his machine keeps happily
> resuming with the new patch applied, that would be good.  I will test it
> with APM suspend and SBP-2 circa tomorrow.

No there's more to it than that.  When the ohci controller processes
the bus reset, it loads back values from the config rom into a couple
of ohci registers, specifically BusOptions[1].  If we provide the
wrong value in config_rom[2], the controller will load that value
after reset and we'll typically end up with a 0 in the max_rec field,
which will prevent async receive from working.  So byteswapping this
correctly will very likely fix the problem you saw, where your box
came back from suspend, but the firewire stack was busted.

cheers,
Kristian

[1] If you look at the bus reset tasklet, you'll see that there's more
to it.  I turns out that not all controllers agree on whether or not
to byteswap the be32 value read from the config_rom map.
Specifically, the ricoh chipset in my laptop gets it right and swaps
it from be32 to le32 before writing to the ohci register, but all TI
chipsets just writes the value as is, effectively making the otherwise
well designed atomic config rom update mechanism useless.  So in the
end I manually write the BusOptions register in the bus reset tasklet,
but I also load it from the ohci->config_rom map, so this has to be
the right byte order.

Re: fw-ohci breaks suspend-to-RAM

[Marcus Better]

[-- Attachment #1.1: Type: text/plain, Size: 232 bytes --]

Stefan Richter wrote:
> Marcus could take the time to check whether his machine keeps happily
> resuming with the new patch applied, that would be good.

Yes it works, but I didn't check firewire functionality after resume.

Marcus

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]