Patchwork [4/5] pata: Update experimental tags

login
register
mail settings
Submitter Alan Cox
Date Nov. 19, 2009, 6:21 p.m.
Message ID <20091119182132.5155f819@lxorguk.ukuu.org.uk>
Download mbox | patch
Permalink /patch/38864/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Alan Cox - Nov. 19, 2009, 6:21 p.m.
> 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. 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.

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.
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). The critical bit other
than get the bits the right way around appears to be the 10uS delay.

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. The vendors own driver uses a different process and
    that one appears to produce plausible numbers.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

--
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
Sergei Shtylyov - Nov. 19, 2009, 6:38 p.m.
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
Bartlomiej Zolnierkiewicz - Nov. 19, 2009, 6:58 p.m.
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.. ;)
Bartlomiej Zolnierkiewicz - Nov. 19, 2009, 7:03 p.m.
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.
Alan Cox - Nov. 19, 2009, 7:40 p.m.
>     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
Sergei Shtylyov - Nov. 19, 2009, 9:21 p.m.
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
Sergei Shtylyov - Nov. 20, 2009, 6:20 p.m.
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

Patch

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;