| Message ID | 20091119182132.5155f819@lxorguk.ukuu.org.uk |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Alan Cox wrote: >>Fixed where? I posted the patch as soon as I noticed the problem. > Its not posted because unlike you I don't post patches as soon as I > notice them. I test them first. Which is why for example I discovered the > bug in the drivers/ide one. Did you check the vendor driver and then > stick 40 and 80 wire cables on the system to check the bits on a 3x2N ? > No I didn't think so. You see if you had you'd have discovered something > else. You'd have discovered another bug in the old IDE one. The driver > code for these chips isn't reliable and doesn't work at all in some cases. >>Told me about it? > Yes - or do you only write replies not read them ? > NAK - the patch is inadequate. No, it was. And yours isn't quite. > The procedure in the vendor driver does > appear to work on the newer chips however. > Probably worth double checking > the HPT37x and seeing if it needs the same debounce delays. All vendor drivers I have do call StallExec(10) when detecting cable type, so need to add the delay to pata_hpt37x too. > Give the patch below a test on your HPT3x2N series device. The PCI -> io > access change doesn't appear to matter but is used by the vendor driver. We should then switch all the driver to I/O access across all the driver, not just here. > I've switched to it because we've been burned before with only some of the > I/O space being visible both ways (eg on the 371N). This is not the case anyway. The change is pointless. > The critical bit other > than get the bits the right way around appears to be the 10uS delay. That's *another* issue. BTW I did seem to *not* need the delay on HPT371N. Though worth retesting... > Alan > -- > > commit d27660f440b516f58385649f705b07f10b24da94 > Author: Alan Cox <alan@linux.intel.com> > Date: Thu Nov 19 17:59:26 2009 +0000 > > pata_hpt3x2n: Fix cable detection > > The version inherited from drivers/ide doesn't work on the newer chipsets > at least not reliably. Please don't call it inherited when you have a bug the drivers/ide/ code doesn't have. > The vendors own driver uses a different process and > that one appears to produce plausible numbers. I don't consider such description adequate. It doesn't mention the original bug with reversed bits at all. > Signed-off-by: Alan Cox <alan@linux.intel.com> > > diff --git a/drivers/ata/pata_hpt3x2n.c b/drivers/ata/pata_hpt3x2n.c > index 3d59fe0..d92ad5b 100644 > --- a/drivers/ata/pata_hpt3x2n.c > +++ b/drivers/ata/pata_hpt3x2n.c > @@ -124,16 +124,15 @@ static u32 hpt3x2n_find_mode(struct ata_port *ap, int speed) > static int hpt3x2n_cable_detect(struct ata_port *ap) > { > u8 scr2, ata66; > - struct pci_dev *pdev = to_pci_dev(ap->host->dev); > + void __iomem *base = ap->ioaddr.bmdma_addr; > > - pci_read_config_byte(pdev, 0x5B, &scr2); > - pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01); > - /* Cable register now active */ > - pci_read_config_byte(pdev, 0x5A, &ata66); > - /* Restore state */ > - pci_write_config_byte(pdev, 0x5B, scr2); > + scr2 = ioread8(base + 0x7B); > + iowrite8(scr2 & ~0x01, base + 0x7B); > + udelay(10); /* Debounce */ > + ata66 = ioread8(base + 0x7A); > + iowrite8(scr2, base + 0x7B); > > - if (ata66 & (1 << ap->port_no)) > + if (ata66 & (2 >> ap->port_no)) > return ATA_CBL_PATA40; > else > return ATA_CBL_PATA80; I'd suggest to address the delay by another, separate patch to both pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for bit reversing... 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 Thursday 19 November 2009 19:21:32 Alan Cox wrote: > > Fixed where? I posted the patch as soon as I noticed the problem. > > Its not posted because unlike you I don't post patches as soon as I > notice them. I test them first. Which is why for example I discovered the > bug in the drivers/ide one. Did you check the vendor driver and then > stick 40 and 80 wire cables on the system to check the bits on a 3x2N ? > No I didn't think so. You see if you had you'd have discovered something I did check the vendor driver but I don't have 3x2N to test it so posting patch ASAP to make it possible for other people to verify it was the best course of action and completely justified. Don't you agree? > else. You'd have discovered another bug in the old IDE one. The driver You mean 'another' like yours _three_ year old 'one'? :) > code for these chips isn't reliable and doesn't work at all in some cases. > > > Told me about it? > > Yes - or do you only write replies not read them ? Well, yours have low SNR and I value my time.. > NAK - the patch is inadequate. The procedure in the vendor driver does Patch is completely adequate in what it tries to achieve (fixing three year old problem with testing the bit for the wrong port) and you could have made an incremental fix for 'a second issue' easily. However I'm not into NIH so your patch is also fine and a more complete one. > pata_hpt3x2n: Fix cable detection > > The version inherited from drivers/ide doesn't work on the newer chipsets > at least not reliably. The vendors own driver uses a different process and > that one appears to produce plausible numbers. Well, maybe except how you decided to 'skip' in the patch description the part about how you have managed to introduce a regression three years ago into already unreliable cable detection taken from hpt366.. ;)
On Thursday 19 November 2009 19:38:26 Sergei Shtylyov wrote: > Alan Cox wrote: > > >>Fixed where? I posted the patch as soon as I noticed the problem. > > > Its not posted because unlike you I don't post patches as soon as I > > notice them. I test them first. Which is why for example I discovered the > > bug in the drivers/ide one. Did you check the vendor driver and then > > stick 40 and 80 wire cables on the system to check the bits on a 3x2N ? > > > No I didn't think so. You see if you had you'd have discovered something > > else. You'd have discovered another bug in the old IDE one. The driver > > code for these chips isn't reliable and doesn't work at all in some cases. > > >>Told me about it? > > > Yes - or do you only write replies not read them ? > > > NAK - the patch is inadequate. > > No, it was. And yours isn't quite. > > > The procedure in the vendor driver does > > appear to work on the newer chips however. > > > Probably worth double checking > > the HPT37x and seeing if it needs the same debounce delays. > > All vendor drivers I have do call StallExec(10) when detecting cable > type, so need to add the delay to pata_hpt37x too. Given this I take back my ACK to Alan's patch. > I'd suggest to address the delay by another, separate patch to both > pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for > bit reversing... Yes.
> I'd suggest to address the delay by another, separate patch to both > pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for > bit reversing... If you can confirm the 371N is fine without switching to I/O cycles then I agree entirely with that and I'll push the udelay(10) into all three plus the drivers/ide one unless Bart beats me to it. Alan -- 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
Hello. Alan Cox wrote: >> I'd suggest to address the delay by another, separate patch to both >> pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for >> bit reversing... >> > > If you can confirm the 371N is fine without switching to I/O cycles then > HPT371N datasheet says that the SRC1/2 registers are dual-mapped to PCI and I/O space. Are you paranoid enough to want me to test the drivers on the board with HPT371N too? :-) > I agree entirely with that and I'll push the udelay(10) into all three > I can't say anything about pata_hpt36x, I don't think I have HPT drivers that old... > plus the drivers/ide one unless Bart beats me to it. > I could beat you to it as well. :-) > Alan 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
Hello, I wrote: >> I agree entirely with that and I'll push the udelay(10) into all three > I can't say anything about pata_hpt36x, I don't think I have HPT > drivers that old... No, actually I can. On HPT36x cable detection bit simply isn't multiplexed, so there's no point in debouncing. >> plus the drivers/ide one unless Bart beats me to it. > I could beat you to it as well. :-) Working in it right now... >> Alan 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
diff --git a/drivers/ata/pata_hpt3x2n.c b/drivers/ata/pata_hpt3x2n.c index 3d59fe0..d92ad5b 100644 --- a/drivers/ata/pata_hpt3x2n.c +++ b/drivers/ata/pata_hpt3x2n.c @@ -124,16 +124,15 @@ static u32 hpt3x2n_find_mode(struct ata_port *ap, int speed) static int hpt3x2n_cable_detect(struct ata_port *ap) { u8 scr2, ata66; - struct pci_dev *pdev = to_pci_dev(ap->host->dev); + void __iomem *base = ap->ioaddr.bmdma_addr; - pci_read_config_byte(pdev, 0x5B, &scr2); - pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01); - /* Cable register now active */ - pci_read_config_byte(pdev, 0x5A, &ata66); - /* Restore state */ - pci_write_config_byte(pdev, 0x5B, scr2); + scr2 = ioread8(base + 0x7B); + iowrite8(scr2 & ~0x01, base + 0x7B); + udelay(10); /* Debounce */ + ata66 = ioread8(base + 0x7A); + iowrite8(scr2, base + 0x7B); - if (ata66 & (1 << ap->port_no)) + if (ata66 & (2 >> ap->port_no)) return ATA_CBL_PATA40; else return ATA_CBL_PATA80;