[PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Sergei Shtylyov]

The DPLL tuning code always set up it for 66 MHz due to wrong UltraDMA mask
including mode 5 used to check for the necessity of 66 MHz clocking -- This
caused 66 MHz clock to be used for HPT374 chip that does not tolerate it.
While fixing this, also remove PLL mode from the TODO list -- I don't think
it's still relevant item.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
This is against the current Linus tree.
Bob, please test it and report what you'll find out...

 drivers/ata/pata_hpt37x.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/ata/pata_hpt37x.c
===================================================================
--- linux-2.6.orig/drivers/ata/pata_hpt37x.c
+++ linux-2.6/drivers/ata/pata_hpt37x.c
@@ -8,12 +8,10 @@
  * Copyright (C) 1999-2003		Andre Hedrick <andre@linux-ide.org>
  * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
  * Portions Copyright (C) 2003		Red Hat Inc
- * Portions Copyright (C) 2005-2006	MontaVista Software, Inc.
+ * Portions Copyright (C) 2005-2007	MontaVista Software, Inc.
  *
  * TODO
- *	PLL mode
- *	Look into engine reset on timeout errors. Should not be
- *		required.
+ *	Look into engine reset on timeout errors. Should not be	required.
  */
 
 #include <linux/kernel.h>
@@ -26,7 +24,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"pata_hpt37x"
-#define DRV_VERSION	"0.6.7"
+#define DRV_VERSION	"0.6.8"
 
 struct hpt_clock {
 	u8	xfer_speed;
@@ -1092,9 +1090,7 @@ static int hpt37x_init_one(struct pci_de
 		int dpll, adjust;
 
 		/* Compute DPLL */
-		dpll = 2;
-		if (port->udma_mask & 0xE0)
-			dpll = 3;
+		dpll = (port->udma_mask & 0xC0) ? 3 : 2;
 
 		f_low = (MHz[clock_slot] * 48) / MHz[dpll];
 		f_high = f_low + 2;

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Alan Cox]

>  		/* Compute DPLL */
> -		dpll = 2;
> -		if (port->udma_mask & 0xE0)
> -			dpll = 3;
> +		dpll = (port->udma_mask & 0xC0) ? 3 : 2;

Gak, I'd much rather people kept to the nice easy to read if() but fine

is_author_p()?Signed-off-by:Acked-by Alan Cox <alan@redhat.com>

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Jeff Garzik]

Alan Cox wrote:
>>  		/* Compute DPLL */
>> -		dpll = 2;
>> -		if (port->udma_mask & 0xE0)
>> -			dpll = 3;
>> +		dpll = (port->udma_mask & 0xC0) ? 3 : 2;
> 
> Gak, I'd much rather people kept to the nice easy to read if() but fine
> 
> is_author_p()?Signed-off-by:Acked-by Alan Cox <alan@redhat.com>

Does your ACK stand, even though it still locks up for Bob Ham?

	Jeff

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Alan Cox]

On Tue, 07 Aug 2007 20:49:34 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> >>  		/* Compute DPLL */
> >> -		dpll = 2;
> >> -		if (port->udma_mask & 0xE0)
> >> -			dpll = 3;
> >> +		dpll = (port->udma_mask & 0xC0) ? 3 : 2;
> > 
> > Gak, I'd much rather people kept to the nice easy to read if() but fine
> > 
> > is_author_p()?Signed-off-by:Acked-by Alan Cox <alan@redhat.com>
> 
> Does your ACK stand, even though it still locks up for Bob Ham?


Its a real fix for a real bug. The 374 needs a bit more stuff which is
sitting in my tree and I'll push for Sergei to comment tomorrow

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Jeff Garzik]

Alan Cox wrote:
> On Tue, 07 Aug 2007 20:49:34 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Alan Cox wrote:
>>>>  		/* Compute DPLL */
>>>> -		dpll = 2;
>>>> -		if (port->udma_mask & 0xE0)
>>>> -			dpll = 3;
>>>> +		dpll = (port->udma_mask & 0xC0) ? 3 : 2;
>>> Gak, I'd much rather people kept to the nice easy to read if() but fine
>>>
>>> is_author_p()?Signed-off-by:Acked-by Alan Cox <alan@redhat.com>
>> Does your ACK stand, even though it still locks up for Bob Ham?
> 
> 
> Its a real fix for a real bug. The 374 needs a bit more stuff which is
> sitting in my tree and I'll push for Sergei to comment tomorrow


OK, thanks.  Do you also ACK

[PATCH 2/2] pata_hpt{37x|3x2n}: fix clock reporting

?

	Jeff

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Alan Cox]

> > Its a real fix for a real bug. The 374 needs a bit more stuff which is
> > sitting in my tree and I'll push for Sergei to comment tomorrow
> 
> 
> OK, thanks.  Do you also ACK
> 
> [PATCH 2/2] pata_hpt{37x|3x2n}: fix clock reporting


Yep

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Bob Ham]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Sun, 2007-08-05 at 22:45 +0400, Sergei Shtylyov wrote:
> The DPLL tuning code always set up it for 66 MHz due to wrong UltraDMA mask
> including mode 5 used to check for the necessity of 66 MHz clocking -- This
> caused 66 MHz clock to be used for HPT374 chip that does not tolerate it.
> While fixing this, also remove PLL mode from the TODO list -- I don't think
> it's still relevant item.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> ---
> This is against the current Linus tree.
> Bob, please test it and report what you'll find out...

para_hpt37x: bus clock 33MHz, using 50MHz DPLL.
ACPI: PCI Interrupt 0000:00:0d.0[A] -> GSI 16 (level, low) -> IRQ 17
scsi2: pata_hpt37x
scsi3: pata_hpt37x
ata3: PATA max UDMA/100 cmd 0x0001efa0 ctl 0x0001ef9e bmdma 0x0001ec00 irq 17
ata4: PATA max UDMA/100 cmd 0x0001ef90 ctl 0x0001ef9a bmdma 0x0001ec00 irq 17


followed by a hard lock

-- 
Bob Ham <rah@bash.sh>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Sergei Shtylyov]

Bob Ham wrote:

>>The DPLL tuning code always set up it for 66 MHz due to wrong UltraDMA mask
>>including mode 5 used to check for the necessity of 66 MHz clocking -- This
>>caused 66 MHz clock to be used for HPT374 chip that does not tolerate it.
>>While fixing this, also remove PLL mode from the TODO list -- I don't think
>>it's still relevant item.

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>---
>>This is against the current Linus tree.
>>Bob, please test it and report what you'll find out...

> para_hpt37x: bus clock 33MHz, using 50MHz DPLL.

    Aha, note that 33 MHz PCI clock is now reported.

> ACPI: PCI Interrupt 0000:00:0d.0[A] -> GSI 16 (level, low) -> IRQ 17
> scsi2: pata_hpt37x
> scsi3: pata_hpt37x
> ata3: PATA max UDMA/100 cmd 0x0001efa0 ctl 0x0001ef9e bmdma 0x0001ec00 irq 17
> ata4: PATA max UDMA/100 cmd 0x0001ef90 ctl 0x0001ef9a bmdma 0x0001ec00 irq 17

> followed by a hard lock

    Well, so it's tougher than just that but the patches are good anyway. 
Wait... doesn't it get stuck trying to pick up the HPT374 chip's function 1?

MBR, Sergei

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Bartlomiej Zolnierkiewicz]

While at it:

On Sunday 05 August 2007, Sergei Shtylyov wrote:

> Index: linux-2.6/drivers/ata/pata_hpt37x.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/pata_hpt37x.c
> +++ linux-2.6/drivers/ata/pata_hpt37x.c
> @@ -8,12 +8,10 @@

this driver is lacking author/maintainer info:

/*
 * Libata driver for the highpoint 37x and 30x UDMA66 ATA controllers.
 *
 * This driver is heavily based upon:
 *
 * linux/drivers/ide/pci/hpt366.c               Version 0.36    April 25, 2003
 *

>   * Copyright (C) 1999-2003		Andre Hedrick <andre@linux-ide.org>
>   * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
>   * Portions Copyright (C) 2003		Red Hat Inc
> - * Portions Copyright (C) 2005-2006	MontaVista Software, Inc.
> + * Portions Copyright (C) 2005-2007	MontaVista Software, Inc.

reference to hpt366 version should also be updated (or removed)

>   *
>   * TODO
> - *	PLL mode
> - *	Look into engine reset on timeout errors. Should not be
> - *		required.
> + *	Look into engine reset on timeout errors. Should not be	required.
>   */
>  
>  #include <linux/kernel.h>

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Alan Cox]

>  * This driver is heavily based upon:
>  *
>  * linux/drivers/ide/pci/hpt366.c               Version 0.36    April 25, 2003
>  *
> reference to hpt366 version should also be updated (or removed)


Disagree - its not based on the newer hpt366 driver. Its based on the old
one. Actually by now it bears almost no resemblence any of them

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Bartlomiej Zolnierkiewicz]

On Friday 10 August 2007, Alan Cox wrote:
> >  * This driver is heavily based upon:
> >  *
> >  * linux/drivers/ide/pci/hpt366.c               Version 0.36    April 25, 2003
> >  *
> > reference to hpt366 version should also be updated (or removed)
> 
> 
> Disagree - its not based on the newer hpt366 driver. Its based on the old

But this patch has just updated copyright references to match with the new
driver while the version still matches the old one!

> one. Actually by now it bears almost no resemblence any of them

Indeed, unfortunately...

Bart

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Alan Cox]

> > one. Actually by now it bears almost no resemblence any of them
> 
> Indeed, unfortunately...

Oh I think its very fortunate. The original IDE one is 3 semi-related
drivers in one file all falling over one another. At least we can now
break them individually 8)

Alan

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>>one. Actually by now it bears almost no resemblence any of them

>>Indeed, unfortunately...

> Oh I think its very fortunate. The original IDE one is 3 semi-related
> drivers in one file all falling over one another. At least we can now
> break them individually 8)

    I'm still thinking that only HPT36x chips were worth moving into another 
driver -- couldn't help myself not saying that once more. :-)
    Curse on HighPoint for their stupid chips, device IDs, and drivers. >:-Q

> Alan

WBR, Sergei

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>> * This driver is heavily based upon:
>>> *
>>> * linux/drivers/ide/pci/hpt366.c               Version 0.36    April 25, 2003
>>> *
>>>reference to hpt366 version should also be updated (or removed)

>>Disagree - its not based on the newer hpt366 driver. Its based on the old

> But this patch has just updated copyright references to match with the new
> driver while the version still matches the old one!

    Err... I've updated the copyright to reflect that I've done some changes 
to these drivers as well...

>>one. Actually by now it bears almost no resemblence any of them

> Indeed, unfortunately...

    :-)

> Bart

MBR, Sergei

Re: [PATCH 1/2] pata_hpt37x: actually clock HPT374 by 50 MHz DPLL

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>Index: linux-2.6/drivers/ata/pata_hpt37x.c
>>===================================================================
>>--- linux-2.6.orig/drivers/ata/pata_hpt37x.c
>>+++ linux-2.6/drivers/ata/pata_hpt37x.c
>>@@ -8,12 +8,10 @@

> this driver is lacking author/maintainer info:

    Why I'm seeing it there then? :-O

> /*
>  * Libata driver for the highpoint 37x and 30x UDMA66 ATA controllers.

    They're actually not UDMA66, they are UDMA100/133.

MBR, Sergei