Allow (forced) AHCI mode on Intel 5 series, 3400 series chipsets

Allow (forced) AHCI mode on Intel 5 series, 3400 series chipsets

[Bernhard Froemel]

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

Hello,

I'd like to submit the following patch for discussion.
Credits go to Greg White (pushing me with his patch into the right
direction, https://bugs.launchpad.net/mactel-support/+bug/817017 ) and
Joerg Dorchain (who found an elegant way to fix this without introducing
any new hacks to the existing framework,
http://ns.spinics.net/lists/linux-ide/msg40206.html ).

Sole purpose of this patch against Linux 3.0 is to enable AHCI mode of
the SATA controller within certain Intel 5 series/3400 series chipset
based systems.

The EFI/bios on the Apple Macbook Pro 6 series (and other Intel Chipset
based Macs and Macbook (Pros)) is ignorant of the SATA controller
AHCI/IDE mode settings. It is not possible to configure the EFI/bios to
set the SATA controller into the AHCI mode and even if the controller is
put into AHCI mode (e.g. by the bootloader) it is not correctly restored
during sleep/suspend-wakeup cycles. The attached patch attempts to fix
this by
1) registering the actual SATA controller mode and/or forcibly set it to
AHCI mode (kernel command parameter: i5s_3400s_force_ahci=1) during
system boot
2) restoring the AHCI mode after wakeup depending on the mode the
controller was during system boot

The patch should have no impact on systems where an EFI/bios takes care
about setting and correctly restoring the SATA controller mode: For
those systems the kernel command parameter 'i5s_3400s_force_ahci=1' only
acts as an additional feature to override the actual EFI/bios setting.

Cheers,
 Bernhard

[-- Attachment #2: i5s_3400s_ahci_handling_for_incapable_efi.patch --]
[-- Type: text/x-patch, Size: 2998 bytes --]

Signed-off-by: Bernhard Froemel <froemel@vmars.tuwien.ac.at>

--- a/drivers/pci/quirks.c	2011-07-22 04:17:23.000000000 +0200
+++ b/drivers/pci/quirks.c	2011-07-28 13:30:04.816817416 +0200
@@ -2799,6 +2799,82 @@
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_TI, 0xb800, fixup_ti816x_class);
 
+/*
+ * Workaround for EFIs/Bios that do not allow to set (and/or restore upon
+ * resume) the SATA controller AHCI mode within the Intel 5 Series/3400
+ * Series chipset. The SATA controller mode is registered during the boot
+ * process. In case of a resume, these fixups check the current controller
+ * mode against the earlier registered one and will restore it to AHCI
+ * mode if necessary. The user can force AHCI by setting the kernel
+ * command parameter 'i5s_3400s_force_ahci=1'.
+ *
+ * The PCI device id (and register contents) change during mode switch, so
+ * these fixups are attached to the LPC and expect that:
+ * -) the SATA controller is on 00:1f.2
+ * -) the SATA controller device is initialized/resumed *after* the LPC device
+*/
+
+#define I5S_REG_AMAP		0x90
+#define I5S_PREFIX			"[i5s_3400s ** FIXUP] "
+static int i5s_3400s_force_ahci;
+
+static int __init i5s_3400s_force_ahci_setup(char *str)
+{
+	if (!strcmp(str, "1"))
+		i5s_3400s_force_ahci = 1;
+	return 0;
+}
+early_param("i5s_3400s_force_ahci", i5s_3400s_force_ahci_setup);
+
+static void __devinit i5s_3400s_fixup(struct pci_dev *dev)
+{
+	u16 amap;
+	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+		I5S_REG_AMAP, &amap);
+	if (amap & 0x60) {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: AHCI (0x%04x)\n", amap);
+		i5s_3400s_force_ahci = 1;
+	} else {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
+		if (i5s_3400s_force_ahci && (amap & 0xE0) == 0) {
+			amap |= 0x60;
+			printk(KERN_DEBUG I5S_PREFIX
+				"Putting SATA controller into AHCI (0x%04x)\n",
+				amap);
+			pci_bus_write_config_word(dev->bus,
+				PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+				I5S_REG_AMAP, amap);
+		}
+	}
+}
+static void i5s_3400s_fixup_resume(struct pci_dev *dev)
+{
+	u16 amap;
+	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+		I5S_REG_AMAP, &amap);
+	if (!(amap & 0x60)) {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
+		amap &= ~BIT(7);
+		amap |= 0x60;
+	  printk(KERN_DEBUG I5S_PREFIX
+			"Restoring AHCI mode of SATA controller (0x%04x)\n",
+			amap);
+		/* MAP - Address Map Register:  AHCI mode + 6 SATA ports */
+		pci_bus_write_config_word(dev->bus,
+			PCI_DEVFN(PCI_SLOT(dev->devfn), 2), I5S_REG_AMAP,
+			amap);
+	} else {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: AHCI (0x%04x)\n", amap);
+	}
+}
+DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, 0x3b09,
+	i5s_3400s_fixup_resume);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3b09, i5s_3400s_fixup);
+
 static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
 			  struct pci_fixup *end)
 {

Re: Allow (forced) AHCI mode on Intel 5 series, 3400 series chipsets

[Tejun Heo]

Hello,

On Sat, Aug 06, 2011 at 10:07:21PM +0200, Bernhard Froemel wrote:
> The EFI/bios on the Apple Macbook Pro 6 series (and other Intel Chipset
> based Macs and Macbook (Pros)) is ignorant of the SATA controller
> AHCI/IDE mode settings. It is not possible to configure the EFI/bios to
> set the SATA controller into the AHCI mode and even if the controller is
> put into AHCI mode (e.g. by the bootloader) it is not correctly restored
> during sleep/suspend-wakeup cycles. The attached patch attempts to fix
> this by
> 1) registering the actual SATA controller mode and/or forcibly set it to
> AHCI mode (kernel command parameter: i5s_3400s_force_ahci=1) during
> system boot
> 2) restoring the AHCI mode after wakeup depending on the mode the
> controller was during system boot
> 
> The patch should have no impact on systems where an EFI/bios takes care
> about setting and correctly restoring the SATA controller mode: For
> those systems the kernel command parameter 'i5s_3400s_force_ahci=1' only
> acts as an additional feature to override the actual EFI/bios setting.
...
> +static int __init i5s_3400s_force_ahci_setup(char *str)
> +{
> +	if (!strcmp(str, "1"))
> +		i5s_3400s_force_ahci = 1;
> +	return 0;
> +}
> +early_param("i5s_3400s_force_ahci", i5s_3400s_force_ahci_setup);

Just setting variable if the parameter is specified would be enough.

> +static void __devinit i5s_3400s_fixup(struct pci_dev *dev)
> +{
> +	u16 amap;
> +	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
> +		I5S_REG_AMAP, &amap);
> +	if (amap & 0x60) {
> +		printk(KERN_DEBUG I5S_PREFIX
> +			"SATA controller mode: AHCI (0x%04x)\n", amap);
> +		i5s_3400s_force_ahci = 1;
> +	} else {
> +		printk(KERN_DEBUG I5S_PREFIX
> +			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
> +		if (i5s_3400s_force_ahci && (amap & 0xE0) == 0) {
> +			amap |= 0x60;
> +			printk(KERN_DEBUG I5S_PREFIX
> +				"Putting SATA controller into AHCI (0x%04x)\n",
> +				amap);
> +			pci_bus_write_config_word(dev->bus,
> +				PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
> +				I5S_REG_AMAP, amap);
> +		}
> +	}
> +}
> +static void i5s_3400s_fixup_resume(struct pci_dev *dev)
> +{
> +	u16 amap;
> +	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
> +		I5S_REG_AMAP, &amap);
> +	if (!(amap & 0x60)) {
> +		printk(KERN_DEBUG I5S_PREFIX
> +			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
> +		amap &= ~BIT(7);
> +		amap |= 0x60;
> +	  printk(KERN_DEBUG I5S_PREFIX
> +			"Restoring AHCI mode of SATA controller (0x%04x)\n",
> +			amap);
> +		/* MAP - Address Map Register:  AHCI mode + 6 SATA ports */
> +		pci_bus_write_config_word(dev->bus,
> +			PCI_DEVFN(PCI_SLOT(dev->devfn), 2), I5S_REG_AMAP,
> +			amap);
> +	} else {
> +		printk(KERN_DEBUG I5S_PREFIX
> +			"SATA controller mode: AHCI (0x%04x)\n", amap);
> +	}
> +}

Shouldn't the resume fixup also check force_ahci?  It looks like it
would put the controller into ahci mode even if it was in ide mode
during boot.

At any rate, unless there's a broken BIOS which boots into ahci mode
but resumes into ide mode, I personally don't feel too enthusiastic
about forcing the controller into ahci mode depending on a kernel
paramster.  If it can be decided automatically and enabled by default,
for sure.  If not, it's not gonna be all that useful anyway.

Thanks.

-- 
tejun

Re: Allow (forced) AHCI mode on Intel 5 series, 3400 series chipsets

[Bernhard Froemel]

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

Hello,
On 08/08/2011 12:07 PM, Tejun Heo wrote:
> Hello,
> 
Thanks for your comments!

> On Sat, Aug 06, 2011 at 10:07:21PM +0200, Bernhard Froemel wrote:
>> +static int __init i5s_3400s_force_ahci_setup(char *str)
>> +{
>> +	if (!strcmp(str, "1"))
>> +		i5s_3400s_force_ahci = 1;
>> +	return 0;
>> +}
>> +early_param("i5s_3400s_force_ahci", i5s_3400s_force_ahci_setup);
> 
> Just setting variable if the parameter is specified would be enough.
I wanted to catch cases like 'i5s_3400s_force_ahci=0', but I can see
that this is probably not a problem.

>> +static void i5s_3400s_fixup_resume(struct pci_dev *dev)
>> [...]
>> +}
> 
> Shouldn't the resume fixup also check force_ahci?  It looks like it
> would put the controller into ahci mode even if it was in ide mode
> during boot.
Right you are; I have no idea how that ended up wrong - I should
probably really don't make any changes after I tested the patch.
Attached the revised and tested patch.

> 
> At any rate, unless there's a broken BIOS which boots into ahci mode
> but resumes into ide mode, I personally don't feel too enthusiastic
> about forcing the controller into ahci mode depending on a kernel
> paramster.  
I renamed the force variable and it should be clearer now:

case a) EFI/bios is not broken and correctly restores the SATA
controller mode before control is given back to the kernel:
The patch has no influence beside when the user decides to supply the
kernel parameter '': Then this patch will put the SATA controller into
AHCI mode during boot and will correctly handle resume.

case b) EFI/bios does not allow to set AHCI and/or does not restore AHCI
(e.g., Apple systems based on that chipset) on resume: The patch enables
AHCI usage on those systems. Without the patch you could previously only
put the SATA controller into AHCI by means of a bootloader -- but then
you still can't resume, because the controller is in the wrong mode
after resume.

> If it can be decided automatically and enabled by default,
> for sure.  If not, it's not gonna be all that useful anyway.
Unfortunately, we'd need to e.g. evaluate DMI info to limit the patch to
systems which are known to not provide an AHCI configuration option in
there EFI/bios. Also we still need some means to tell the kernel whether
to put the SATA controller into AHCI or remain in the default (IDE)
mode: a kernel command parameter crossed my mind first.

Additional to my original submission I ensured to only set the
controller into AHCI mode when it's according to the datasheet 'safe'.

Thanks,
  Bernhard

[-- Attachment #2: i5s_3400s_ahci_handling_for_incapable_efi.patch --]
[-- Type: text/x-patch, Size: 3168 bytes --]

Signed-off-by: Bernhard Froemel <froemel@vmars.tuwien.ac.at>
--- a/drivers/pci/quirks.c	2011-07-22 04:17:23.000000000 +0200
+++ b/drivers/pci/quirks.c	2011-08-08 13:37:54.459428437 +0200
@@ -2799,6 +2799,87 @@
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_TI, 0xb800, fixup_ti816x_class);
 
+/*
+ * Workaround for EFIs/Bios that do not allow to set (and/or restore upon
+ * resume) the SATA controller AHCI mode within the Intel 5 Series/3400
+ * Series chipset. The SATA controller mode is registered during the boot
+ * process. In case of a resume, these fixups check the current controller
+ * mode against the earlier registered one and will restore it to AHCI
+ * mode if necessary. The user can force AHCI by setting the kernel
+ * command parameter 'i5s_3400s_force_ahci=1'.
+ *
+ * The PCI device id (and register contents) change during mode switch, so
+ * these fixups are attached to the LPC and expect that:
+ * -) the SATA controller is on 00:1f.2
+ * -) the SATA controller device is initialized/resumed *after* the LPC device
+*/
+
+#define I5S_REG_AMAP		0x90
+#define I5S_PREFIX			"[i5s_3400s ** FIXUP] "
+static int i5s_3400s_sata_ahci;
+
+static int __init i5s_3400s_force_ahci_setup(char *str)
+{
+	i5s_3400s_sata_ahci = 1;
+	return 0;
+}
+early_param("i5s_3400s_force_ahci", i5s_3400s_force_ahci_setup);
+
+static void __devinit i5s_3400s_fixup(struct pci_dev *dev)
+{
+	u16 amap;
+	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+		I5S_REG_AMAP, &amap);
+	if (amap & 0x60) {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: AHCI (0x%04x)\n", amap);
+		i5s_3400s_sata_ahci = 1;
+	} else {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
+		/* only honor i5s_3400s_sata ahci if the controller is
+		in a mode were it's safe (IDE Mode, MV == 0x0) to
+		switch to AHCI */
+		if (i5s_3400s_sata_ahci && (amap & 0xC3) == 0) {
+			amap |= 0x60;
+			printk(KERN_DEBUG I5S_PREFIX
+				"Putting SATA controller into AHCI (0x%04x)\n",
+				amap);
+			pci_bus_write_config_word(dev->bus,
+				PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+				I5S_REG_AMAP, amap);
+		}
+	}
+}
+static void i5s_3400s_fixup_resume(struct pci_dev *dev)
+{
+	u16 amap;
+	pci_bus_read_config_word(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+		I5S_REG_AMAP, &amap);
+	if (!(amap & 0x60)) {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: NON AHCI (0x%04x)\n", amap);
+		if (i5s_3400s_sata_ahci && (amap & 0xc3) == 0) {
+			amap &= ~BIT(7);
+			amap |= 0x60;
+			printk(KERN_DEBUG I5S_PREFIX
+				"Restoring AHCI mode of SATA controller (0x%04x)\n",
+				amap);
+			/* MAP - Address Map Register:  AHCI mode + 6 SATA
+			ports */
+			pci_bus_write_config_word(dev->bus,
+				PCI_DEVFN(PCI_SLOT(dev->devfn), 2),
+				I5S_REG_AMAP, amap);
+		}
+	} else {
+		printk(KERN_DEBUG I5S_PREFIX
+			"SATA controller mode: AHCI (0x%04x)\n", amap);
+	}
+}
+DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, 0x3b09,
+	i5s_3400s_fixup_resume);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3b09, i5s_3400s_fixup);
+
 static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
 			  struct pci_fixup *end)
 {

Re: Allow (forced) AHCI mode on Intel 5 series, 3400 series chipsets

[Bernhard Froemel]

Hello,

are there any more objections with this patch [1]? or do I need to
resubmit to a specific person?
I'd really like to get it into the kernel ;)

Cheers,
 Bernhard

[1] http://www.spinics.net/lists/linux-ide/msg41438.html