| Message ID | 20110211173620.GF5778@Redstar.dorchain.net |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
--- 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;