lkml.org 
[lkml]   [2006]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: Linux v2.6.18-rc5
    Hello, Keith Owens.

    Sorry about late respond. I'm quite occupied with personal stuff these
    days.

    Keith Owens wrote:
    >>> (2) I have seen the same intermittent bug on ICH7 SATA but
    >>> PIIX_FLAG_IGNORE_PCS is only set for ich5 and i6300esb_sata. It
    >>> probably needs to be set for ich7 as well.
    >> No, ICH7 up to this point has been believed to have well-behaving PCS.
    >> If you report PCS problem, you'll be the first. Also, note that ICH7
    >> suffers from ghost device probing problem if PCS is not honored exactly.
    >> Are you sure it's the same problem?
    >
    > It definitely looks like it. Stock 2.6.18-rc5 plus this patch to
    > activate ata_debug from boot until just after probing drives.
    [--snip--]
    > <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0
    > <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x15 present_mask=0x3
    > <3>piix_sata_present_mask: ata1: ENTER, pcs=0x0 base=0
    > <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x0 present_mask=0x3
    > <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0
    > <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x15 present_mask=0x3
    > <3>piix_sata_present_mask: ata1: ENTER, pcs=0x0 base=0
    > <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x0 present_mask=0x3
    > <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0

    Yeah, it definitely looks like it. Does the kernel print a message
    which looks like the following before those debug messages?

    "updating PCS from 0x0 to 0x5"

    > Note the pcs=0x0 values. Adding PIIX_FLAG_IGNORE_PCS to
    > ich6m_sata_ahci gets past the failure to detect pcs, with no sign of
    > any ghost devices. BTW, dropping down to 2.6.17 with the same config
    > has no problem detecting the disk, even without PIIX_FLAG_IGNORE_PCS on
    > ich6m_sata_ahci.

    BTW, you can set PIIX_FLAG_IGNORE_PCS by using force_pcs=1 module
    parameter now.

    > lspci extract, this is an ICH7M.
    >
    > 00:1f.0 Class 0601: 8086:27b9 (rev 02)
    > Subsystem: 1033:832c
    > Flags: bus master, medium devsel, latency 0
    > Capabilities: [e0] Vendor Specific Information
    >
    > 00:1f.2 Class 0101: 8086:27c4 (rev 02) (prog-if 80)
    > Subsystem: 1033:832c
    > Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 18
    > I/O ports at <unassigned>
    > I/O ports at <unassigned>
    > I/O ports at <unassigned>
    > I/O ports at <unassigned>
    > I/O ports at 18b0 [size=16]
    > Capabilities: [70] Power Management version 2
    >
    > 00:1f.3 Class 0c05: 8086:27da (rev 02)
    > Subsystem: 1033:832c
    > Flags: medium devsel, IRQ 11
    > I/O ports at 18c0 [size=32]
    >

    Hmm... Can you try the attached patch and see what happens? ATM, I'm on
    the road and can't test the patch, so it's only compile-tested. This
    patch basically reverts some of the effects of the following commit and
    makes PCS update a little bit more aggressive iff necessary.

    ea35d29e2fa8b3d766a2ce8fbcce599dce8d2734
    [libata] ata_piix: Consolidate PCS register writing

    If this works for you ich7m, can you please test this on your formerly
    problematic ich5 with force_pcs=2 specified? I initially thought that
    the ich5 problem was caused by exact PCS map change and thus added
    IGNORE_PCS as workaround but if the same problem occurs on ich7 and is
    fixed by the attached patch, it's due to conservative PCS update change
    and thus the original IGNORE_PCS fix on ich5 might not be necessary.

    Sorry this PCS business causes you and other people so much trouble. It
    just doesn't work quite as ata_piix developers expect.

    Thanks.

    --
    tejun
    diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
    index 2d20caf..46f7c9b 100644
    --- a/drivers/scsi/ata_piix.c
    +++ b/drivers/scsi/ata_piix.c
    @@ -553,15 +553,42 @@ static unsigned int piix_sata_present_ma
    {
    struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
    struct piix_host_priv *hpriv = ap->host_set->private_data;
    + const struct piix_map_db *map_db = hpriv->map_db;
    const unsigned int *map = hpriv->map;
    int base = 2 * ap->hard_port_no;
    unsigned int present_mask = 0;
    int port, i;
    - u16 pcs;
    + u16 pcs, new_pcs;

    pci_read_config_word(pdev, ICH5_PCS, &pcs);
    DPRINTK("ata%u: ENTER, pcs=0x%x base=%d\n", ap->id, pcs, base);

    + new_pcs = pcs | map_db->port_enable;
    +
    + if (pcs != new_pcs) {
    + u16 old_pcs = pcs;
    +
    + for (i = 0; i < 10; i++) {
    + pci_write_config_word(pdev, ICH5_PCS, new_pcs);
    + msleep(150);
    + pci_read_config_word(pdev, ICH5_PCS, &pcs);
    +
    + new_pcs = pcs | map_db->port_enable;
    + if (pcs == new_pcs)
    + break;
    + }
    +
    + if (pcs == new_pcs)
    + ata_port_printk(ap, KERN_INFO, "updated PCS from "
    + "0x%x to 0x%x (%d tries)\n",
    + old_pcs, pcs, i);
    + else
    + ata_port_printk(ap, KERN_WARNING,
    + "failed to update PCS after %d tries, "
    + "old=0x%x cur=0x%x new=0x%x\n",
    + i, old_pcs, pcs, new_pcs);
    + }
    +
    for (i = 0; i < 2; i++) {
    port = map[base + i];
    if (port < 0)
    @@ -816,35 +843,6 @@ static int __devinit piix_check_450nx_er
    return no_piix_dma;
    }

    -static void __devinit piix_init_pcs(struct pci_dev *pdev,
    - struct ata_port_info *pinfo,
    - const struct piix_map_db *map_db)
    -{
    - u16 pcs, new_pcs;
    -
    - pci_read_config_word(pdev, ICH5_PCS, &pcs);
    -
    - new_pcs = pcs | map_db->port_enable;
    -
    - if (new_pcs != pcs) {
    - DPRINTK("updating PCS from 0x%x to 0x%x\n", pcs, new_pcs);
    - pci_write_config_word(pdev, ICH5_PCS, new_pcs);
    - msleep(150);
    - }
    -
    - if (force_pcs == 1) {
    - dev_printk(KERN_INFO, &pdev->dev,
    - "force ignoring PCS (0x%x)\n", new_pcs);
    - pinfo[0].host_flags |= PIIX_FLAG_IGNORE_PCS;
    - pinfo[1].host_flags |= PIIX_FLAG_IGNORE_PCS;
    - } else if (force_pcs == 2) {
    - dev_printk(KERN_INFO, &pdev->dev,
    - "force honoring PCS (0x%x)\n", new_pcs);
    - pinfo[0].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
    - pinfo[1].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
    - }
    -}
    -
    static void __devinit piix_init_sata_map(struct pci_dev *pdev,
    struct ata_port_info *pinfo,
    const struct piix_map_db *map_db)
    @@ -893,6 +891,17 @@ static void __devinit piix_init_sata_map

    hpriv->map = map;
    hpriv->map_db = map_db;
    +
    + /* handle force_pcs module parameter */
    + if (force_pcs == 1) {
    + dev_printk(KERN_INFO, &pdev->dev, "force ignoring PCS\n");
    + pinfo[0].host_flags |= PIIX_FLAG_IGNORE_PCS;
    + pinfo[1].host_flags |= PIIX_FLAG_IGNORE_PCS;
    + } else if (force_pcs == 2) {
    + dev_printk(KERN_INFO, &pdev->dev, "force honoring PCS\n");
    + pinfo[0].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
    + pinfo[1].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
    + }
    }

    /**
    @@ -948,12 +957,9 @@ static int piix_init_one (struct pci_dev
    }

    /* Initialize SATA map */
    - if (host_flags & ATA_FLAG_SATA) {
    + if (host_flags & ATA_FLAG_SATA)
    piix_init_sata_map(pdev, port_info,
    piix_map_db_table[ent->driver_data]);
    - piix_init_pcs(pdev, port_info,
    - piix_map_db_table[ent->driver_data]);
    - }

    /* On ICH5, some BIOSen disable the interrupt using the
    * PCI_COMMAND_INTX_DISABLE bit added in PCI 2.3.
    \
     
     \ /
      Last update: 2006-09-03 20:47    [W:2.367 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site