Re: [PATCH 4/5] pata: Update experimental tags

Sergei Shtylyov
2009-11-19T18:37:47Z

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