[Patch] Enable AHCI on certain ich chipsets

[Patch] Enable AHCI on certain ich chipsets

[Joerg Dorchain]

[-- Attachment #1: Type: text/plain, Size: 4085 bytes --]

Hello all,

Disclaimer: this patch is based on http://mjg59.livejournal.com/85504.html
It is in works-for-me state.

The patch to ahci.c is required for suspend/resume.

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-09 11:16:36.000000000 +0100
@@ -2684,6 +2684,79 @@
 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 bool ich_ahci_mode_forced = 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;
+        /* const u32 ich89_func_disable_reg_offset = 0x3418; */
+
+	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);
+                ich_ahci_mode_forced = true;
+                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-04 18:23:41.000000000 +0100
@@ -640,6 +640,8 @@
 	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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]

Re: [Patch] Enable AHCI on certain ich chipsets

[Sergei Shtylyov]

Hello.

On 09-02-2011 14:59, Joerg Dorchain wrote:

> Hello all,

> Disclaimer: this patch is based on http://mjg59.livejournal.com/85504.html
> It is in works-for-me state.

> The patch to ahci.c is required for suspend/resume.

    Need a better description than thatl, I think...

> 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-09 11:16:36.000000000 +0100
> @@ -2684,6 +2684,79 @@
>   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 bool ich_ahci_mode_forced = false;

    This variable is write-only, hence not needed.

> +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;
> +        /* const u32 ich89_func_disable_reg_offset = 0x3418; */

    No commented out code, please.

> +        /* 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))
> +{

    Brace should be on the same line as the end of *if*.

> --- linux/drivers/ata/ahci.c.orig	2011-02-04 18:13:33.000000000 +0100
> +++ linux/drivers/ata/ahci.c	2011-02-04 18:23:41.000000000 +0100
> @@ -640,6 +640,8 @@
>   	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

    No C99 // comments please.

WBR, Sergei

Re: [Patch] Enable AHCI on certain ich chipsets

[Joerg Dorchain]

[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]

Hello all,

On Wed, Feb 09, 2011 at 03:56:32PM +0300, Sergei Shtylyov wrote:
[Several formal corrections]

Should be addressed by this try.

> >The patch to ahci.c is required for suspend/resume.
> 
>    Need a better description than thatl, I think...

Well, then it is a bit longer story.

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. 

Admittingly, more testing would be welcome.


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-09 14:40:15.000000000 +0100
@@ -2684,6 +2684,76 @@
 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-09 14:30:06.000000000 +0100
@@ -640,6 +640,9 @@
 	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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]

Re: [Patch] Enable AHCI on certain ich chipsets

[Sergei Shtylyov]

Hello.

On 10-02-2011 22:23, Joerg Dorchain wrote:

> On Wed, Feb 09, 2011 at 03:56:32PM +0300, Sergei Shtylyov wrote:
> [Several formal corrections]

> Should be addressed by this try.

>>> The patch to ahci.c is required for suspend/resume.

>>     Need a better description than thatl, I think...

> Well, then it is a bit longer story.

> 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.

> Admittingly, more testing would be welcome.

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

    This only describes drivers/ata/ahci.c change. And looks like it should be 
in a patch of its own...

> --- linux/drivers/pci/quirks.c.orig	2011-02-04 18:29:03.000000000 +0100
> +++ linux/drivers/pci/quirks.c	2011-02-09 14:40:15.000000000 +0100
> @@ -2684,6 +2684,76 @@
[...]
> +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;

    You indent with spaces instead of tab here.

> +
> +	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))

    You again indent with spaces...

> +	{

    I've clearly said that { should be on the same line as the end of *if* 
statement.

> +                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);
> +                ich_ahci_mode_forced = true;
> +                dev_printk(KERN_DEBUG, &pdev->dev, "Forced ICH7/8/9 mode PIIX->AHCI\n");

    Again indented with spaces...

> +        }
> +

    Empty line not needed...

> +}

> --- linux/drivers/ata/ahci.c.orig	2011-02-04 18:13:33.000000000 +0100
> +++ linux/drivers/ata/ahci.c	2011-02-09 14:30:06.000000000 +0100
> @@ -640,6 +640,9 @@
>   	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 */

    The preferred style for the multi-line comments is this:

/*
  * bla
  * bla
  */

WBR, Sergei

Re: [Patch] Enable AHCI on certain ich chipsets

[Joerg Dorchain]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

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>

--- 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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]

Re: [Patch] Enable AHCI on certain ich chipsets

[Sergei Shtylyov]

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

Re: [Patch] Enable AHCI on certain ich chipsets

[Joerg Dorchain]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]

Re: [Patch] Enable AHCI on certain ich chipsets

[Bartlomiej Zolnierkiewicz]

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

Re: [Patch] Enable AHCI on certain ich chipsets

[Joerg Dorchain]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]