Patchwork Enable AHCI on certain ich chipsets

login
register
mail settings
Submitter Joerg Dorchain
Date Feb. 11, 2011, 5:36 p.m.
Message ID <20110211173620.GF5778@Redstar.dorchain.net>
Download mbox | patch
Permalink /patch/82793/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Joerg Dorchain - Feb. 11, 2011, 5:36 p.m.
On Fri, Feb 11, 2011 at 03:27:04PM +0300, Sergei Shtylyov wrote:
> >[Several formal corrections]
> 
> >Should be addressed by this try.

Thank you for bearing with me. Next try is below.
> 
> >During resume from suspend to ram, the kernel pci layer restores
> >the registers for the SATA controller once, then says okay, and
> >sets dev->state_saved = false. However, since the restore goes
> >from highest address (the BARs [base address registers]) to
> >lowest register, some of the higher registers are set as RO
> >because according to the lower registers controller is in PIIX
> >mode.  This patch introduces a workaround for
> >this problem, hacking around the PCI API by setting pdev->state_saved = true
> >before we do the restore.
> 
> 
> 
> This only describes drivers/ata/ahci.c change.

Well, the functionality of the patch to quirks.c is described in
the comments on the top of it. Should that be repeated?

> And looks like it
> should be in a patch of its own...

I need both parts in order to use the AHCI driver and having
suspend/resume work, hence they are together.

Bye,

Joerg


Signed-Off-By: joerg Dorchain<joerg@dorchain.net>
Sergei Shtylyov - Feb. 11, 2011, 8:50 p.m.
Hello.

Joerg Dorchain wrote:

>>> During resume from suspend to ram, the kernel pci layer restores
>>> the registers for the SATA controller once, then says okay, and
>>> sets dev->state_saved = false. However, since the restore goes
>> >from highest address (the BARs [base address registers]) to
>>> lowest register, some of the higher registers are set as RO
>>> because according to the lower registers controller is in PIIX
>>> mode.  This patch introduces a workaround for
>>> this problem, hacking around the PCI API by setting pdev->state_saved = true
>>> before we do the restore.

>> This only describes drivers/ata/ahci.c change.

> Well, the functionality of the patch to quirks.c is described in
> the comments on the top of it. Should that be repeated?

    Yes, certainly.

>> And looks like it
>> should be in a patch of its own...

> I need both parts in order to use the AHCI driver and having
> suspend/resume work, hence they are together.

    They are solving two different issues, as far as I understand, hence should 
be in two different patches. One issue per patch is the basic rule.

> Bye,

> Joerg

> Signed-Off-By: joerg Dorchain<joerg@dorchain.net>

> --- linux/drivers/pci/quirks.c.orig	2011-02-04 18:29:03.000000000 +0100
> +++ linux/drivers/pci/quirks.c	2011-02-11 13:44:12.000000000 +0100
> @@ -2684,6 +2684,74 @@
[...]
> +static void ich789_force_ahci_mode(struct pci_dev *pdev)
> +{
> +	u8 amrval;
> +	u8 sclkgc;
> +	const int ich89_address_map_reg = 0x90;
> +	const int ich89_sata_clock_gen_config_reg = 0x9c;
> +
> +	if (!ich_force_ahci_mode)
> +		return;
> +
> +	/* ICH8 datasheet section 12.1.33 */
> +	if (!pci_bus_read_config_byte(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 2),
> +        	ich89_address_map_reg, &amrval)) {

    Still spaces at the start of line... run your patches thru 
scripts/checkpatch.pl -- it notices such style violations.

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joerg Dorchain - Feb. 12, 2011, 6:27 a.m.
On Fri, Feb 11, 2011 at 11:50:06PM +0300, Sergei Shtylyov wrote:
> 
> >Well, the functionality of the patch to quirks.c is described in
> >the comments on the top of it. Should that be repeated?
> 
>    Yes, certainly.

Ok, will do so.
> 
> >>And looks like it
> >>should be in a patch of its own...
> 
> >I need both parts in order to use the AHCI driver and having
> >suspend/resume work, hence they are together.
> 
> 
> They are solving two different issues, as far as I understand, hence
> should be in two different patches. One issue per patch is the basic
> rule.

Here I am not sure. Suspend/resume works fine (for me) without
any of the patches. The first patch fiddles with pci config space
in order to achieve something that  which should be done by the
BIOS, hence it is in quirks, together with other workarounds.

The second part just makes sure that the config space change is
kept after a resume. Otherwise it comes up as a different PCI
id, the AHCI driver finds nothing to work with, the harddisk is
gone after resume, not good.

You could also see it as it triggers a general fault, and
should be fixed anyway indepent of the trigger case.

Whereas I am quite sure about the quirks part, I'd be glad for
some functional review of the ahci part, to avoid undesired side
effects.
> 
> 
> scripts/checkpatch.pl -- it notices such style violations.

Thank you for that hint.

Bye,

Joerg
Bartlomiej Zolnierkiewicz - Feb. 12, 2011, 12:09 p.m.
Hi,

On Sat, Feb 12, 2011 at 7:27 AM, Joerg Dorchain <joerg@dorchain.net> wrote:

> The second part just makes sure that the config space change is
> kept after a resume. Otherwise it comes up as a different PCI
> id, the AHCI driver finds nothing to work with, the harddisk is
> gone after resume, not good.

Shouldn't therefore the quirk be applied also during resume (by
additional use of DECLARE_PCI_FIXUP_RESUME_EARLY in addition to
existing DECLARE_PCI_FIXUP_EARLY one)?

Thanks,
Bartomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joerg Dorchain - Feb. 14, 2011, 7:41 a.m.
On Sat, Feb 12, 2011 at 01:09:37PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> > The second part just makes sure that the config space change is
> > kept after a resume. Otherwise it comes up as a different PCI
> > id, the AHCI driver finds nothing to work with, the harddisk is
> > gone after resume, not good.
> 
> Shouldn't therefore the quirk be applied also during resume (by
> additional use of DECLARE_PCI_FIXUP_RESUME_EARLY in addition to
> existing DECLARE_PCI_FIXUP_EARLY one)?

I have considered that, but just not touching the config space
for having the desired effect seems easier to me.

Actually this is a reason why I am looking for feedback from
other people with the chipsets listed in the patch. Is it only my
system for keeps pci config during suspend/resume or does it work
for others, too?
On the other hand, if I understand Sergei correctly, it could be
a more general effect with it, which is why I'd also appreciated
some review from an architectural perspective.

If not, DECLARE_PCI_FIXUP_RESUME_EARLY is fine with me. My main
objective is to use the ahci driver, and I hope others like to do
so as well.

Bye,

Joerg

Patch

--- linux/drivers/pci/quirks.c.orig	2011-02-04 18:29:03.000000000 +0100
+++ linux/drivers/pci/quirks.c	2011-02-11 13:44:12.000000000 +0100
@@ -2684,6 +2684,74 @@ 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_HINT, 0x0020, quirk_hotplug_bridge);
 
 /*
+ * Force ICH7/8/9 into AHCI mode.  This is needed because some
+ * BIOSes do not make AHCI-mode operation available to the user.
+ * As the Intel documentation states that the OS should not carry
+ * out the operation - the user must force this on the kernel
+ * commandline using quirk_ich_force_ahci
+ *
+ * As this quirk gets called whilst the PCI subsystem is
+ * walking the PCI bus, we declare this quirk against the LPC
+ * (device 00:1f.0), so that we can frob 00:1f.2 before the PCI
+ * code has scanned it.
+ * Note: the pci id might change due to this (e.g. from 27c4 to 27c5)
+ *
+ */
+
+static bool ich_force_ahci_mode = false;
+
+static int __init ich789_force_ahci_mode_setup(char *str)
+{
+	ich_force_ahci_mode = true;
+	return 0;
+}
+early_param("quirk_ich_force_ahci", ich789_force_ahci_mode_setup);
+
+static void ich789_force_ahci_mode(struct pci_dev *pdev)
+{
+	u8 amrval;
+	u8 sclkgc;
+	const int ich89_address_map_reg = 0x90;
+	const int ich89_sata_clock_gen_config_reg = 0x9c;
+
+	if (!ich_force_ahci_mode)
+		return;
+
+	/* ICH8 datasheet section 12.1.33 */
+	if (!pci_bus_read_config_byte(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 2),
+        	ich89_address_map_reg, &amrval)) {
+
+		if (amrval & (BIT(6) | BIT(7))) {
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"ICH7/8/9 SATA controller not in IDE mode.  Not modifying.\n");
+			return;
+		}
+		if (amrval & (BIT(0) | BIT(1)))
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"ICH7/8/9 in SATA/PATA combined mode.  Untested.\n");
+		/* AHCI mode */
+		amrval |= BIT(6);
+		amrval &= ~BIT(7);
+		pci_bus_read_config_byte(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 2), 
+			ich89_sata_clock_gen_config_reg, &sclkgc);
+		dev_printk(KERN_DEBUG, &pdev->dev, "sclkgc is %#0x\n", sclkgc);
+		pci_bus_write_config_byte(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 2), 
+			ich89_address_map_reg, amrval);
+		dev_printk(KERN_DEBUG, &pdev->dev, "Forced ICH7/8/9 mode PIIX->AHCI\n");
+	}
+}
+/* ICH7 */
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x27b9, ich789_force_ahci_mode);
+/* ICH8 */
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH8_0, ich789_force_ahci_mode);
+/* ICH9R LPC */
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2916, ich789_force_ahci_mode);
+/* ICH9M LPC */
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2917, ich789_force_ahci_mode);
+/* ICH9M-E LPC */
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2919, ich789_force_ahci_mode);
+
+/*
  * This is a quirk for the Ricoh MMC controller found as a part of
  * some mulifunction chips.
 
--- linux/drivers/ata/ahci.c.orig	2011-02-04 18:13:33.000000000 +0100
+++ linux/drivers/ata/ahci.c	2011-02-11 13:45:22.000000000 +0100
@@ -640,6 +640,11 @@ 
 	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 	int rc;
 
+	/*
+	 * override check to see if PCI config space is already
+	 * restored in pci_restore_state
+	 */
+	pdev->state_saved = true;
 	rc = ata_pci_device_do_resume(pdev);
 	if (rc)
 		return rc;