Re: [PATCH v1] ata_piix: attempt to fix

[unicorn chang]

I test this patch on IBM Intellistation M Pro with Quantum GoVault
(kernel 2.6.18-rc1),
the "long boot delay" and "ghost devices" are still existing.

I also test tejun's patch for "ghost devices", it's worked for me.
(http://marc.theaimsgroup.com/?l=linux-ide&m=115164294506130&w=2)

SATA controller is configured to "Compatible" mode.
Intel Corporation 82801GB/GR/GH (ICH7 Family) Serial ATA Storage
Controllers cc=IDE (rev 01)

Unicorn

----------  dmesg
---------------------------------------------------------------------------------------------------------
ata_piix 0000:00:1f.2: MAP [ P0 P2 P1 P3 ]
piix_init_pcs: updating PCS from 0x33 to 0x3f
ata_pci_init_one: ENTER
Device `[IDE1]' is not power manageable<6>ACPI: PCI Interrupt
0000:00:1f.2[B] -> GSI 19 (level, low) -> IRQ 19
PCI: Setting latency timer of device 0000:00:1f.2 to 64
ata_device_add: ENTER
ata_host_add: ENTER
ata_port_start: prd alloc, virt dfa03000, dma 1fa03000
ata3: SATA max UDMA/133 cmd 0x3828 ctl 0x381E bmdma 0x34F0 irq 19
__ata_port_freeze: ata3 port frozen
ata_host_add: ENTER
ata_port_start: prd alloc, virt dfa93000, dma 1fa93000
ata4: SATA max UDMA/133 cmd 0x3820 ctl 0x381A bmdma 0x34F8 irq 19
__ata_port_freeze: ata4 port frozen
ata_device_add: probe begin
scsi2 : ata_piix
PM: Adding info for No Bus:host2
ata_port_schedule_eh: port EH scheduled
ata_scsi_error: ENTER
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata3: ata_port_flush_task: flush #2
ata3: ata_port_flush_task: EXIT
ata_eh_autopsy: ENTER
ata_eh_recover: ENTER
ata_eh_prep_resume: ENTER
ata_eh_prep_resume: EXIT
__ata_port_freeze: ata3 port frozen
piix_sata_prereset: ata3: ENTER, pcs=0x3f base=0
piix_sata_prereset: ata3: LEAVE, pcs=0x3f present_mask=0x1
ata_std_softreset: ENTER
ata_std_softreset: about to softreset, devmask=3
ata_bus_softreset: ata3: bus reset via SRST
ata_dev_classify: found ATA device by sig
ata_dev_classify: found ATA device by sig
ata_std_softreset: EXIT, classes[0]=1 [1]=5
ata_std_postreset: ENTER
ata_std_postreset: EXIT
ata_eh_thaw_port: ata3 port thawed
ata_eh_revalidate_and_attach: ENTER
ata3.00: ata_dev_read_id: ENTER, host 3, dev 0
ata_pio_sector: data read
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata3: ata_port_flush_task: flush #2
ata3: ata_port_flush_task: EXIT
ata3.00: ATA-7, max UDMA/133, 156312576 sectors: LBA48 NCQ (depth 0/32)
ata3.00: ata3: dev 0 multi count 16
ata_eh_revalidate_and_attach: EXIT
ata_eh_resume: ENTER
ata_eh_resume: EXIT
ata_dev_set_xfermode: set features - xfer mode
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata3: ata_port_flush_task: flush #2
ata3: ata_port_flush_task: EXIT
ata_dev_set_xfermode: EXIT, err_mask=0
ata3.00: ata_dev_read_id: ENTER, host 3, dev 0
ata_pio_sector: data read
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata3: ata_port_flush_task: flush #2
ata3: ata_port_flush_task: EXIT
ata_dev_set_mode: xfer_shift=8, xfer_mode=0x46
ata3.00: configured for UDMA/133
ata_eh_suspend: ENTER
ata_eh_suspend: EXIT
ata_eh_recover: EXIT, rc=0
ata_scsi_error: EXIT
scsi3 : ata_piix
PM: Adding info for No Bus:host3
ata_port_schedule_eh: port EH scheduled
ata_scsi_error: ENTER
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata4: ata_port_flush_task: flush #2
ata4: ata_port_flush_task: EXIT
ata_eh_autopsy: ENTER
ata_eh_recover: ENTER
ata_eh_prep_resume: ENTER
ata_eh_prep_resume: EXIT
__ata_port_freeze: ata4 port frozen
piix_sata_prereset: ata4: ENTER, pcs=0x3f base=2
piix_sata_prereset: ata4: LEAVE, pcs=0x3f present_mask=0x1
ata_std_softreset: ENTER
ata_std_softreset: about to softreset, devmask=3
ata_bus_softreset: ata4: bus reset via SRST
ata_dev_classify: found ATAPI device by sig
ata_dev_classify: found ATAPI device by sig
ata_std_softreset: EXIT, classes[0]=3 [1]=3
ata_std_postreset: ENTER
ata_std_postreset: EXIT
ata_eh_thaw_port: ata4 port thawed
ata_eh_revalidate_and_attach: ENTER
ata4.00: ata_dev_read_id: ENTER, host 4, dev 0
ata_exec_command_pio: ata4: cmd 0xA1
ata_hsm_move: ata4: protocol 2 task_state 2 (dev_stat 0x58)
ata_pio_sector: data read
ata_hsm_move: ata4: protocol 2 task_state 3 (dev_stat 0x50)
ata_hsm_move: ata4: dev 0 command complete, drv_stat 0x50
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata4: ata_port_flush_task: flush #2
ata4: ata_port_flush_task: EXIT
ata4.00: ATAPI, max UDMA/66, CDB intr
ata4.01: ata_dev_read_id: ENTER, host 4, dev 1
ata_exec_command_pio: ata4: cmd 0xA1
ata_port_flush_task: ENTER
ata_port_flush_task: flush #1
ata4: ata_port_flush_task: flush #2
ata4: ata_port_flush_task: EXIT
__ata_port_freeze: ata4 port frozen
ata4.01: qc timeout (cmd 0xa1)
ata4.01: failed to IDENTIFY (I/O error, err_mask=0x4)
ata_eh_revalidate_and_attach: EXIT
ata4: failed to recover some devices, retrying in 5 secs



On 7/12/06, Jeff Garzik <jeff@xxxxxxxxxx> wrote:
> This patch attempts to address problems on ata_piix that people have
> been reporting:  long boot delay, ghost devices, and ICH8 brokenness.
>
> Testing feedback is requested as soon as possible, so that we can
> potentially stick this into 2.6.18-rc2.
>
> I'm booting up several boxes locally here, mainly ICH5 and ICH8, as well.
>
>
>  drivers/scsi/ata_piix.c |  151 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 106 insertions(+), 45 deletions(-)
>
>
> commit aeff1cdc5e86f0ce6267c1ae76d150064e05faf6
> Author: Jeff Garzik <jeff@xxxxxxxxxx>
> Date:   Tue Jul 11 11:57:44 2006 -0400
>
>     [libata] ata_piix: attempt to fix ICH8 support
>
>     Take into account the fact that ICH8 changed the register layout of
>     the MAP and PCS register bits.
>
>     Signed-off-by: Jeff Garzik <jeff@xxxxxxxxxx>
>
> commit 15dcf5b9868382026c268529d2bb922db676cf63
> Author: Jeff Garzik <jeff@xxxxxxxxxx>
> Date:   Tue Jul 11 11:48:50 2006 -0400
>
>     [libata] ata_piix: Consolidate PCS register writing
>
>     Prior to this patch, the driver would do this for each port:
>         read 8-bit PCS
>         write 8-bit PCS
>         read 8-bit PCS
>         write 8-bit PCS
>
>     In the field, flaky behavior has been observed related to this register.
>     In particular, these overzealous register writes can cause misdetection
>     problems.
>
>     Update to do the following once (not once per port) at boot:
>         read 16-bit PCS
>         if needs changing,
>                 write 16-bit PCS
>
>     And thereafter, we only perform a 'read 16-bit PCS' per port.
>
>     This should eliminate all PCS writes in many cases, and be more friendly
>     in the cases where we do need to enable ports.
>
>     Signed-off-by: Jeff Garzik <jeff@xxxxxxxxxx>
>
> commit 38c7dbfd2883dccfdeaaa22cd809561000577908
> Author: Tejun Heo <htejun@xxxxxxxxx>
> Date:   Thu Jun 29 01:58:28 2006 +0900
>
>     [PATCH] ata_piix: add host_set private structure
>
>     Add host_set private structure piix_host_priv.  Currently the only
>     field is ->map which used to be stored directly at
>     host_set->private_data.  This change allows more host_set private
>     fields to be added.
>
>     Signed-off-by: Tejun Heo <htejun@xxxxxxxxx>
>     Signed-off-by: Jeff Garzik <jeff@xxxxxxxxxx>
>
>
>
> diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
> index 94b1261..a960bb7 100644
> --- a/drivers/scsi/ata_piix.c
> +++ b/drivers/scsi/ata_piix.c
> @@ -126,6 +126,7 @@ enum {
>         ich6_sata               = 4,
>         ich6_sata_ahci          = 5,
>         ich6m_sata_ahci         = 6,
> +       ich8_sata_ahci          = 7,
>
>         /* constants for mapping table */
>         P0                      = 0,  /* port 0 */
> @@ -141,11 +142,19 @@ enum {
>
>  struct piix_map_db {
>         const u32 mask;
> +       const u32 port_enable;
> +       const int present_shift;
>         const int map[][4];
>  };
>
> +struct piix_host_priv {
> +       const int *map;
> +       const struct piix_map_db *map_db;
> +};
> +
>  static int piix_init_one (struct pci_dev *pdev,
>                                     const struct pci_device_id *ent);
> +static void piix_host_stop(struct ata_host_set *host_set);
>  static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
>  static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
>  static void piix_pata_error_handler(struct ata_port *ap);
> @@ -186,11 +195,11 @@ #endif
>         /* Enterprise Southbridge 2 (where's the datasheet?) */
>         { 0x8086, 0x2680, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
>         /* SATA Controller 1 IDE (ICH8, no datasheet yet) */
> -       { 0x8086, 0x2820, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
> +       { 0x8086, 0x2820, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_ahci },
>         /* SATA Controller 2 IDE (ICH8, ditto) */
> -       { 0x8086, 0x2825, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
> +       { 0x8086, 0x2825, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_ahci },
>         /* Mobile SATA Controller IDE (ICH8M, ditto) */
> -       { 0x8086, 0x2828, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6m_sata_ahci },
> +       { 0x8086, 0x2828, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_ahci },
>
>         { }     /* terminate list */
>  };
> @@ -254,7 +263,7 @@ static const struct ata_port_operations
>
>         .port_start             = ata_port_start,
>         .port_stop              = ata_port_stop,
> -       .host_stop              = ata_host_stop,
> +       .host_stop              = piix_host_stop,
>  };
>
>  static const struct ata_port_operations piix_sata_ops = {
> @@ -284,11 +293,13 @@ static const struct ata_port_operations
>
>         .port_start             = ata_port_start,
>         .port_stop              = ata_port_stop,
> -       .host_stop              = ata_host_stop,
> +       .host_stop              = piix_host_stop,
>  };
>
> -static struct piix_map_db ich5_map_db = {
> +static const struct piix_map_db ich5_map_db = {
>         .mask = 0x7,
> +       .port_enable = 0x3,
> +       .present_shift = 4,
>         .map = {
>                 /* PM   PS   SM   SS       MAP  */
>                 {  P0,  NA,  P1,  NA }, /* 000b */
> @@ -302,8 +313,10 @@ static struct piix_map_db ich5_map_db =
>         },
>  };
>
> -static struct piix_map_db ich6_map_db = {
> +static const struct piix_map_db ich6_map_db = {
>         .mask = 0x3,
> +       .port_enable = 0xf,
> +       .present_shift = 4,
>         .map = {
>                 /* PM   PS   SM   SS       MAP */
>                 {  P0,  P2,  P1,  P3 }, /* 00b */
> @@ -313,8 +326,10 @@ static struct piix_map_db ich6_map_db =
>         },
>  };
>
> -static struct piix_map_db ich6m_map_db = {
> +static const struct piix_map_db ich6m_map_db = {
>         .mask = 0x3,
> +       .port_enable = 0x5,
> +       .present_shift = 4,
>         .map = {
>                 /* PM   PS   SM   SS       MAP */
>                 {  P0,  P2,  RV,  RV }, /* 00b */
> @@ -324,6 +339,28 @@ static struct piix_map_db ich6m_map_db =
>         },
>  };
>
> +static const struct piix_map_db ich8_map_db = {
> +       .mask = 0x3,
> +       .port_enable = 0x3,
> +       .present_shift = 8,
> +       .map = {
> +               /* PM   PS   SM   SS       MAP */
> +               {  P0,  RV,  P1,  RV }, /* 00b (hardwired) */
> +               {  RV,  RV,  RV,  RV },
> +               {  RV,  RV,  RV,  RV }, /* 10b (never) */
> +               {  RV,  RV,  RV,  RV },
> +       },
> +};
> +
> +static const struct piix_map_db *piix_map_db_table[] = {
> +       [ich5_sata]             = &ich5_map_db,
> +       [esb_sata]              = &ich5_map_db,
> +       [ich6_sata]             = &ich6_map_db,
> +       [ich6_sata_ahci]        = &ich6_map_db,
> +       [ich6m_sata_ahci]       = &ich6m_map_db,
> +       [ich8_sata_ahci]        = &ich8_map_db,
> +};
> +
>  static struct ata_port_info piix_port_info[] = {
>         /* piix4_pata */
>         {
> @@ -362,7 +399,6 @@ #endif
>                 .mwdma_mask     = 0x07, /* mwdma0-2 */
>                 .udma_mask      = 0x7f, /* udma0-6 */
>                 .port_ops       = &piix_sata_ops,
> -               .private_data   = &ich5_map_db,
>         },
>
>         /* i6300esb_sata */
> @@ -374,7 +410,6 @@ #endif
>                 .mwdma_mask     = 0x07, /* mwdma0-2 */
>                 .udma_mask      = 0x7f, /* udma0-6 */
>                 .port_ops       = &piix_sata_ops,
> -               .private_data   = &ich5_map_db,
>         },
>
>         /* ich6_sata */
> @@ -386,7 +421,6 @@ #endif
>                 .mwdma_mask     = 0x07, /* mwdma0-2 */
>                 .udma_mask      = 0x7f, /* udma0-6 */
>                 .port_ops       = &piix_sata_ops,
> -               .private_data   = &ich6_map_db,
>         },
>
>         /* ich6_sata_ahci */
> @@ -399,7 +433,6 @@ #endif
>                 .mwdma_mask     = 0x07, /* mwdma0-2 */
>                 .udma_mask      = 0x7f, /* udma0-6 */
>                 .port_ops       = &piix_sata_ops,
> -               .private_data   = &ich6_map_db,
>         },
>
>         /* ich6m_sata_ahci */
> @@ -412,7 +445,18 @@ #endif
>                 .mwdma_mask     = 0x07, /* mwdma0-2 */
>                 .udma_mask      = 0x7f, /* udma0-6 */
>                 .port_ops       = &piix_sata_ops,
> -               .private_data   = &ich6m_map_db,
> +       },
> +
> +       /* ich8_sata_ahci */
> +       {
> +               .sht            = &piix_sht,
> +               .host_flags     = ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
> +                                 PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
> +                                 PIIX_FLAG_AHCI,
> +               .pio_mask       = 0x1f, /* pio0-4 */
> +               .mwdma_mask     = 0x07, /* mwdma0-2 */
> +               .udma_mask      = 0x7f, /* udma0-6 */
> +               .port_ops       = &piix_sata_ops,
>         },
>  };
>
> @@ -508,46 +552,29 @@ static void piix_pata_error_handler(stru
>  static int piix_sata_prereset(struct ata_port *ap)
>  {
>         struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
> -       const unsigned int *map = ap->host_set->private_data;
> +       struct piix_host_priv *hpriv = ap->host_set->private_data;
> +       const unsigned int *map = hpriv->map;
>         int base = 2 * ap->hard_port_no;
> -       unsigned int present_mask = 0;
> +       unsigned int present = 0;
>         int port, i;
> -       u8 pcs;
> +       u16 pcs;
>
> -       pci_read_config_byte(pdev, ICH5_PCS, &pcs);
> +       pci_read_config_word(pdev, ICH5_PCS, &pcs);
>         DPRINTK("ata%u: ENTER, pcs=0x%x base=%d\n", ap->id, pcs, base);
>
> -       /* enable all ports on this ap and wait for them to settle */
> -       for (i = 0; i < 2; i++) {
> -               port = map[base + i];
> -               if (port >= 0)
> -                       pcs |= 1 << port;
> -       }
> -
> -       pci_write_config_byte(pdev, ICH5_PCS, pcs);
> -       msleep(100);
> -
> -       /* let's see which devices are present */
> -       pci_read_config_byte(pdev, ICH5_PCS, &pcs);
> -
>         for (i = 0; i < 2; i++) {
>                 port = map[base + i];
>                 if (port < 0)
>                         continue;
> -               if (ap->flags & PIIX_FLAG_IGNORE_PCS || pcs & 1 << (4 + port))
> -                       present_mask |= 1 << i;
> -               else
> -                       pcs &= ~(1 << port);
> +               if ((ap->flags & PIIX_FLAG_IGNORE_PCS) ||
> +                   (pcs & 1 << (hpriv->map_db->present_shift + port)))
> +                       present = 1;
>         }
>
> -       /* disable offline ports on non-AHCI controllers */
> -       if (!(ap->flags & PIIX_FLAG_AHCI))
> -               pci_write_config_byte(pdev, ICH5_PCS, pcs);
> -
>         DPRINTK("ata%u: LEAVE, pcs=0x%x present_mask=0x%x\n",
>                 ap->id, pcs, present_mask);
>
> -       if (!present_mask) {
> +       if (!present) {
>                 ata_port_printk(ap, KERN_INFO, "SATA port has no device.\n");
>                 ap->eh_context.i.action &= ~ATA_EH_RESET_MASK;
>                 return 0;
> @@ -761,10 +788,27 @@ static int __devinit piix_check_450nx_er
>         return no_piix_dma;
>  }
>
> +static void __devinit piix_init_pcs(struct pci_dev *pdev,
> +                                   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);
> +       }
> +}
> +
>  static void __devinit piix_init_sata_map(struct pci_dev *pdev,
> -                                        struct ata_port_info *pinfo)
> +                                        struct ata_port_info *pinfo,
> +                                        const struct piix_map_db *map_db)
>  {
> -       struct piix_map_db *map_db = pinfo[0].private_data;
> +       struct piix_host_priv *hpriv = pinfo[0].private_data;
>         const unsigned int *map;
>         int i, invalid_map = 0;
>         u8 map_value;
> @@ -805,8 +849,8 @@ static void __devinit piix_init_sata_map
>                 dev_printk(KERN_ERR, &pdev->dev,
>                            "invalid MAP value %u\n", map_value);
>
> -       pinfo[0].private_data = (void *)map;
> -       pinfo[1].private_data = (void *)map;
> +       hpriv->map = map;
> +       hpriv->map_db = map_db;
>  }
>
>  /**
> @@ -829,6 +873,7 @@ static int piix_init_one (struct pci_dev
>         static int printed_version;
>         struct ata_port_info port_info[2];
>         struct ata_port_info *ppinfo[2] = { &port_info[0], &port_info[1] };
> +       struct piix_host_priv *hpriv;
>         unsigned long host_flags;
>
>         if (!printed_version++)
> @@ -839,8 +884,14 @@ static int piix_init_one (struct pci_dev
>         if (!in_module_init)
>                 return -ENODEV;
>
> +       hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
> +       if (!hpriv)
> +               return -ENOMEM;
> +
>         port_info[0] = piix_port_info[ent->driver_data];
>         port_info[1] = piix_port_info[ent->driver_data];
> +       port_info[0].private_data = hpriv;
> +       port_info[1].private_data = hpriv;
>
>         host_flags = port_info[0].host_flags;
>
> @@ -855,8 +906,11 @@ static int piix_init_one (struct pci_dev
>         }
>
>         /* Initialize SATA map */
> -       if (host_flags & ATA_FLAG_SATA)
> -               piix_init_sata_map(pdev, port_info);
> +       if (host_flags & ATA_FLAG_SATA) {
> +               piix_init_sata_map(pdev, port_info,
> +                                  piix_map_db_table[ent->driver_data]);
> +               piix_init_pcs(pdev, 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.
> @@ -879,6 +933,13 @@ static int piix_init_one (struct pci_dev
>         return ata_pci_init_one(pdev, ppinfo, 2);
>  }
>
> +static void piix_host_stop(struct ata_host_set *host_set)
> +{
> +       if (host_set->next == NULL)
> +               kfree(host_set->private_data);
> +       ata_host_stop(host_set);
> +}
> +
>  static int __init piix_init(void)
>  {
>         int rc;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/