| Message ID | 1303254709.11237.34.camel@mulgrave.site (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
> + struct pci_dev *bridge = pdev->bus->self; > + /* mobility split bridges don't report enabled ports correctly */ > + int port_ok = !(bridge && bridge->vendor == > + PCI_VENDOR_ID_MOBILITY_ELECTRONICS); > The logic seems wrong even by inspection port_ok will be 1 if it isn't a M/E bridge. > + /* check for enabled ports */ > + pci_read_config_byte(pdev, CNTRL, ®); > + if (port_ok) > + dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n"); If its *not* an ME bridge then print the warning ???? Otherwise looks right -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 20-04-2011 3:11, James Bottomley wrote: >> Beats me then. Whatever - its easy enough to work around and avoid >> exploding parisc and sparc so it definitely wants sorting > OK, so are we all agreed on this (I'll split it up into the cosmetic > libata piece and the cmd64x fix later)? > James > --- > diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c > index f8380ce..b1b926c 100644 > --- a/drivers/ata/libata-sff.c > +++ b/drivers/ata/libata-sff.c > @@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host, > return -ENOMEM; > > if (!legacy_mode&& pdev->irq) { > + int i; > + > rc = devm_request_irq(dev, pdev->irq, irq_handler, > IRQF_SHARED, drv_name, host); > if (rc) > goto out; > > - ata_port_desc(host->ports[0], "irq %d", pdev->irq); > - ata_port_desc(host->ports[1], "irq %d", pdev->irq); > + for (i = 0; i < 2; i++) { > + if (ata_port_is_dummy(host->ports[i])) > + continue; > + ata_port_desc(host->ports[i], "irq %d", pdev->irq); Does this really makes any difference? > diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c > index 905ff76..c39fd5a 100644 > --- a/drivers/ata/pata_cmd64x.c > +++ b/drivers/ata/pata_cmd64x.c > @@ -41,6 +41,9 @@ > enum { > CFR = 0x50, > CFR_INTR_CH0 = 0x04, > + CNTRL = 0x51, > + CNTRL_PRIMARY = 0x04, > + CNTRL_SECONDARY = 0x08, Probably better to call them CNTRL_CH0 and CNTRL_CH1 to keep the naming in line with already existing one... > @@ -328,9 +331,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id) > .port_ops =&cmd648_port_ops > } > }; > - const struct ata_port_info *ppi[] = {&cmd_info[id->driver_data], NULL }; > - u8 mrdmode; > + const struct ata_port_info *ppi[] = { > + &cmd_info[id->driver_data], > + &cmd_info[id->driver_data], > + NULL > + }; > + u8 mrdmode, reg; > int rc; > + struct pci_dev *bridge = pdev->bus->self; > + /* mobility split bridges don't report enabled ports correctly */ > + int port_ok = !(bridge && bridge->vendor == > + PCI_VENDOR_ID_MOBILITY_ELECTRONICS); > > rc = pcim_enable_device(pdev); > if (rc) > @@ -354,6 +369,21 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id) > mrdmode |= 0x02; /* Memory read line enable */ > pci_write_config_byte(pdev, MRDMODE, mrdmode); > > + /* check for enabled ports */ > + pci_read_config_byte(pdev, CNTRL,®); > + if (port_ok) You mean !port_ok. > + dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n"); > + /* 643 and 646 no UDMA, primary port always enabled */ > + if (port_ok && id->driver_data > 1 && !(reg & CNTRL_PRIMARY)) { PCI0646U and later revisions on PCI0646 do have the primary port enable bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in cmd64x_init_one()... > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 4e2c915..7a0ac45 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -608,6 +608,8 @@ > #define PCI_DEVICE_ID_MATROX_G550 0x2527 > #define PCI_DEVICE_ID_MATROX_VIA 0x4536 > > +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS 0x14f2 > + The current trend seems to be to only define vendor/device IDs where they are used and not in pci_ids.h... MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-04-20 at 14:04 +0400, Sergei Shtylyov wrote: > > + dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n"); > > + /* 643 and 646 no UDMA, primary port always enabled */ > > + if (port_ok && id->driver_data > 1 && !(reg & CNTRL_PRIMARY)) { > > PCI0646U and later revisions on PCI0646 do have the primary port enable > bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in > cmd64x_init_one()... Where? All I see in drivers/ide/cmd64x.c is that it only ignores the primary for the id->driver_data == 0 case, which is what I originally coded. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 20-04-2011 18:28, James Bottomley wrote: >>> + dev_printk(KERN_NOTICE,&pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n"); >>> + /* 643 and 646 no UDMA, primary port always enabled */ >>> + if (port_ok&& id->driver_data> 1&& !(reg& CNTRL_PRIMARY)) { This should probably be: if (port_ok && !(id->driver_data == 0 || id->driver_data == 1 && pdev->revision < 3) && !(reg & CNTRL_PRIMARY)) { >> PCI0646U and later revisions on PCI0646 do have the primary port enable >> bit. The same about UltraDMA -- PCI0646U2 has it. Look at what cmd64x does in >> cmd64x_init_one()... > Where? All I see in drivers/ide/cmd64x.c is that it only ignores the > primary for the id->driver_data == 0 case, which is what I originally > coded. Hm, are we looking at the same driver? static const struct ide_port_info cmd64x_chipsets[] __devinitdata = { { /* 0: CMD643 */ .name = DRV_NAME, .init_chipset = init_chipset_cmd64x, .enablebits = {{0x00,0x00,0x00}, {0x51,0x08,0x08}}, .port_ops = &cmd64x_port_ops, .host_flags = IDE_HFLAG_CLEAR_SIMPLEX | IDE_HFLAG_ABUSE_PREFETCH | IDE_HFLAG_SERIALIZE, .pio_mask = ATA_PIO5, .mwdma_mask = ATA_MWDMA2, .udma_mask = 0x00, /* no udma */ }, { /* 1: CMD646 */ .name = DRV_NAME, .init_chipset = init_chipset_cmd64x, .enablebits = {{0x51,0x04,0x04}, {0x51,0x08,0x08}}, .port_ops = &cmd648_port_ops, .host_flags = IDE_HFLAG_ABUSE_PREFETCH | IDE_HFLAG_SERIALIZE, .pio_mask = ATA_PIO5, .mwdma_mask = ATA_MWDMA2, .udma_mask = ATA_UDMA2, }, { /* 2: CMD648 */ .name = DRV_NAME, .init_chipset = init_chipset_cmd64x, .enablebits = {{0x51,0x04,0x04}, {0x51,0x08,0x08}}, .port_ops = &cmd648_port_ops, .host_flags = IDE_HFLAG_ABUSE_PREFETCH, .pio_mask = ATA_PIO5, .mwdma_mask = ATA_MWDMA2, .udma_mask = ATA_UDMA4, }, { /* 3: CMD649 */ .name = DRV_NAME, .init_chipset = init_chipset_cmd64x, .enablebits = {{0x51,0x04,0x04}, {0x51,0x08,0x08}}, .port_ops = &cmd648_port_ops, .host_flags = IDE_HFLAG_ABUSE_PREFETCH, .pio_mask = ATA_PIO5, .mwdma_mask = ATA_MWDMA2, .udma_mask = ATA_UDMA5, } }; static int __devinit cmd64x_init_one(struct pci_dev *dev, const struct pci_device_id *id) { struct ide_port_info d; u8 idx = id->driver_data; d = cmd64x_chipsets[idx]; if (idx == 1) { /* * UltraDMA only supported on PCI646U and PCI646U2, which * correspond to revisions 0x03, 0x05 and 0x07 respectively. * Actually, although the CMD tech support people won't * tell me the details, the 0x03 revision cannot support * UDMA correctly without hardware modifications, and even * then it only works with Quantum disks due to some * hold time assumptions in the 646U part which are fixed * in the 646U2. * * So we only do UltraDMA on revision 0x05 and 0x07 chipsets. */ if (dev->revision < 5) { d.udma_mask = 0x00; /* * The original PCI0646 didn't have the primary * channel enable bit, it appeared starting with * PCI0646U (i.e. revision ID 3). */ if (dev->revision < 3) { d.enablebits[0].reg = 0; d.port_ops = &cmd64x_port_ops; if (dev->revision == 1) d.dma_ops = &cmd646_rev1_dma_ops; } } } return ide_pci_init_one(dev, &d, NULL); } static const struct pci_device_id cmd64x_pci_tbl[] = { { PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_643), 0 }, { PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_646), 1 }, { PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_648), 2 }, { PCI_VDEVICE(CMD, PCI_DEVICE_ID_CMD_649), 3 }, { 0, }, }; MODULE_DEVICE_TABLE(pci, cmd64x_pci_tbl); "ïdx == 1" corresponds to PCI0646. See this "dev->revision < 3" check (this is true for the original PCI0646), where it then zeroes the 'reg' field of 'enablebits' to disable its checking? > James WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 20, 2011 at 02:04:10PM +0400, Sergei Shtylyov wrote: >> +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS 0x14f2 >> + > > The current trend seems to be to only define vendor/device IDs where > they are used and not in pci_ids.h... Device IDs, yes. Vendor IDs should always go to the pci_ids.h file, since they're likely to be used in multiple places.
On 04/20/2011 10:56 AM, Matthew Wilcox wrote: > On Wed, Apr 20, 2011 at 02:04:10PM +0400, Sergei Shtylyov wrote: >>> +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS 0x14f2 >>> + >> >> The current trend seems to be to only define vendor/device IDs where >> they are used and not in pci_ids.h... > > Device IDs, yes. Vendor IDs should always go to the pci_ids.h file, since > they're likely to be used in multiple places. Correct; that's the current libata policy. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index f8380ce..b1b926c 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -2447,13 +2447,18 @@ int ata_pci_sff_activate_host(struct ata_host *host, return -ENOMEM; if (!legacy_mode && pdev->irq) { + int i; + rc = devm_request_irq(dev, pdev->irq, irq_handler, IRQF_SHARED, drv_name, host); if (rc) goto out; - ata_port_desc(host->ports[0], "irq %d", pdev->irq); - ata_port_desc(host->ports[1], "irq %d", pdev->irq); + for (i = 0; i < 2; i++) { + if (ata_port_is_dummy(host->ports[i])) + continue; + ata_port_desc(host->ports[i], "irq %d", pdev->irq); + } } else if (legacy_mode) { if (!ata_port_is_dummy(host->ports[0])) { rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev), diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c index 905ff76..c39fd5a 100644 --- a/drivers/ata/pata_cmd64x.c +++ b/drivers/ata/pata_cmd64x.c @@ -41,6 +41,9 @@ enum { CFR = 0x50, CFR_INTR_CH0 = 0x04, + CNTRL = 0x51, + CNTRL_PRIMARY = 0x04, + CNTRL_SECONDARY = 0x08, CMDTIM = 0x52, ARTTIM0 = 0x53, DRWTIM0 = 0x54, @@ -328,9 +331,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id) .port_ops = &cmd648_port_ops } }; - const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL }; - u8 mrdmode; + const struct ata_port_info *ppi[] = { + &cmd_info[id->driver_data], + &cmd_info[id->driver_data], + NULL + }; + u8 mrdmode, reg; int rc; + struct pci_dev *bridge = pdev->bus->self; + /* mobility split bridges don't report enabled ports correctly */ + int port_ok = !(bridge && bridge->vendor == + PCI_VENDOR_ID_MOBILITY_ELECTRONICS); rc = pcim_enable_device(pdev); if (rc) @@ -341,11 +352,15 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id) if (pdev->device == PCI_DEVICE_ID_CMD_646) { /* Does UDMA work ? */ - if (pdev->revision > 4) + if (pdev->revision > 4) { ppi[0] = &cmd_info[2]; + ppi[1] = &cmd_info[2]; + } /* Early rev with other problems ? */ - else if (pdev->revision == 1) + else if (pdev->revision == 1) { ppi[0] = &cmd_info[3]; + ppi[1] = &cmd_info[3]; + } } pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64); @@ -354,6 +369,21 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id) mrdmode |= 0x02; /* Memory read line enable */ pci_write_config_byte(pdev, MRDMODE, mrdmode); + /* check for enabled ports */ + pci_read_config_byte(pdev, CNTRL, ®); + if (port_ok) + dev_printk(KERN_NOTICE, &pdev->dev, "Mobility Bridge detected, ignoring CNTRL port enable/disable\n"); + /* 643 and 646 no UDMA, primary port always enabled */ + if (port_ok && id->driver_data > 1 && !(reg & CNTRL_PRIMARY)) { + dev_printk(KERN_NOTICE, &pdev->dev, "Primary port is disabled\n"); + ppi[0] = &ata_dummy_port_info; + + } + if (port_ok && !(reg & CNTRL_SECONDARY)) { + dev_printk(KERN_NOTICE, &pdev->dev, "Secondary port is disabled\n"); + ppi[1] = &ata_dummy_port_info; + } + /* Force PIO 0 here.. */ /* PPC specific fixup copied from old driver */ diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 4e2c915..7a0ac45 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -608,6 +608,8 @@ #define PCI_DEVICE_ID_MATROX_G550 0x2527 #define PCI_DEVICE_ID_MATROX_VIA 0x4536 +#define PCI_VENDOR_ID_MOBILITY_ELECTRONICS 0x14f2 + #define PCI_VENDOR_ID_CT 0x102c #define PCI_DEVICE_ID_CT_69000 0x00c0 #define PCI_DEVICE_ID_CT_65545 0x00d8