[PATCH] pata_hpt3x2n: Clean up DPLL stuff

[PATCH] pata_hpt3x2n: Clean up DPLL stuff

[Alan Cox]

Nobody commented when I asked for review earlier so it must be ok  8)

Lets stick it in -mm to be sure

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --exclude-from /usr/src/exclude --recursive linux.vanilla-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c linux-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c
--- linux.vanilla-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c	2007-09-26 16:46:48.000000000 +0100
+++ linux-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c	2007-09-18 16:44:32.000000000 +0100
@@ -1,5 +1,5 @@
 /*
- * Libata driver for the highpoint 37x and 30x UDMA66 ATA controllers.
+ * Libata driver for the highpoint 37x and 30x UDMA ATA controllers.
  *
  * This driver is heavily based upon:
  *
@@ -844,6 +844,46 @@
 	/* Never went stable */
 	return 0;
 }
+
+static void *hpt_tune_function(struct pci_dev *dev, int dpll, int clock_slot)
+{
+	static const int MHz[4] = { 33, 40, 50, 66 };
+	/*
+	 *	For non UDMA133 capable devices we should
+	 *	use a 50MHz DPLL by choice
+	 */
+	unsigned int f_low, f_high;
+	int adjust;
+
+	f_low = (MHz[clock_slot] * 48) / MHz[dpll];
+	f_high = f_low + 2;
+	if (clock_slot > 1)
+		f_high += 2;
+	/* Select the DPLL clock. */
+	pci_write_config_byte(dev, 0x5b, 0x21);
+	pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);
+
+	for(adjust = 0; adjust < 8; adjust++) {
+		if (hpt37x_calibrate_dpll(dev))
+			break;
+		/* See if it'll settle at a fractionally different clock */
+		if (adjust & 1)
+			f_low -= adjust >> 1;
+		else
+			f_high += adjust >> 1;
+		pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);
+	}
+	if (adjust == 8) {
+		printk(KERN_WARNING "hpt37x: DPLL did not stabilize.\n");
+		return NULL;
+	}
+	printk(KERN_INFO "hpt37x: Bus clock %dMHz, using DPLL.\n", MHz[dpll]);
+	if (dpll == 3)
+		return hpt37x_timings_66;
+	else
+		return hpt37x_timings_50;
+}
+
 /**
  *	hpt37x_init_one		-	Initialise an HPT37X/302
  *	@dev: PCI device
@@ -944,7 +984,7 @@
 	u8 mcr1;
 	u32 freq;
 	int prefer_dpll = 1;
-
+	int hpt374alt = 0;
 	unsigned long iobase = pci_resource_start(dev, 4);
 
 	const struct hpt_chip *chip_table;
@@ -1046,16 +1086,28 @@
 	if (chip_table == &hpt372a)
 		outb(0x0e, iobase + 0x9c);
 
+	/*
+	 * PLL must be done once
+	 */
+	 
+	if (chip_table == &hpt374 && PCI_FUNC(dev->devfn) & 1) {
+		/* The HPT374 secondary devfn is tuned to 50MHz when we find
+		   the primary */
+		port_info = *port;
+		port_info.private_data = hpt37x_timings_50;
+	}	
 	/* Some devices do not let this value be accessed via PCI space
 	   according to the old driver */
-
 	freq = inl(iobase + 0x90);
 	if ((freq >> 12) != 0xABCDE) {
 		int i;
 		u8 sr;
 		u32 total = 0;
-
+		
 		printk(KERN_WARNING "pata_hpt37x: BIOS has not set timing clocks.\n");
+		if (hpt374alt == 1)
+			printk(KERN_ERR "pata_hpt37x: No saved frequency on primary function.\n");
+
 
 		/* This is the process the HPT371 BIOS is reported to use */
 		for(i = 0; i < 128; i++) {
@@ -1074,48 +1126,19 @@
 
 	clock_slot = hpt37x_clock_slot(freq, chip_table->base);
 	if (chip_table->clocks[clock_slot] == NULL || prefer_dpll) {
-		/*
-		 *	We need to try PLL mode instead
-		 *
-		 *	For non UDMA133 capable devices we should
-		 *	use a 50MHz DPLL by choice
-		 */
-		unsigned int f_low, f_high;
-		int dpll, adjust;
-
 		/* Compute DPLL */
-		dpll = (port->udma_mask & 0xC0) ? 3 : 2;
-
-		f_low = (MHz[clock_slot] * 48) / MHz[dpll];
-		f_high = f_low + 2;
-		if (clock_slot > 1)
-			f_high += 2;
-
-		/* Select the DPLL clock. */
-		pci_write_config_byte(dev, 0x5b, 0x21);
-		pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);
-
-		for(adjust = 0; adjust < 8; adjust++) {
-			if (hpt37x_calibrate_dpll(dev))
-				break;
-			/* See if it'll settle at a fractionally different clock */
-			if (adjust & 1)
-				f_low -= adjust >> 1;
-			else
-				f_high += adjust >> 1;
-			pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);
-		}
-		if (adjust == 8) {
-			printk(KERN_ERR "pata_hpt37x: DPLL did not stabilize!\n");
-			return -ENODEV;
+		int dpll = 2;
+		if (port->udma_mask & 0xC0)
+  			dpll = 3;
+		private_data = hpt_tune_function(dev, dpll, clock_slot);
+		/* For the HPT374 tune both channels together from fn 0 */
+		if (chip_table == &hpt374 && !(PCI_FUNC(dev->devfn) & 1)) {
+			struct pci_dev *pair = pci_get_slot(dev->bus, dev->devfn + 1);
+			if (pair != NULL) {
+				hpt_tune_function(pair, dpll, clock_slot);
+				pci_dev_put(pair);
+			}
 		}
-		if (dpll == 3)
-			private_data = (void *)hpt37x_timings_66;
-		else
-			private_data = (void *)hpt37x_timings_50;
-
-		printk(KERN_INFO "pata_hpt37x: bus clock %dMHz, using %dMHz DPLL.\n",
-		       MHz[clock_slot], MHz[dpll]);
 	} else {
 		private_data = (void *)chip_table->clocks[clock_slot];
 		/*

Re: [PATCH] pata_hpt3x2n: Clean up DPLL stuff

[Jeff Garzik]

Alan Cox wrote:
> Nobody commented when I asked for review earlier so it must be ok  8)
> 
> Lets stick it in -mm to be sure
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

applied to libata-dev.git#for-testing

Re: [PATCH] pata_hpt3x2n: Clean up DPLL stuff

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

> Nobody commented when I asked for review earlier so it must be ok  8)

    It's not that I've seen this before

 > so it must be ok  8)

    Not by me at least, so let me NAK it. 8-)

> Lets stick it in -mm to be sure

    Now let's unstick it. :-)

> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> diff -u --new-file --exclude-from /usr/src/exclude --recursive linux.vanilla-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c linux-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c
> --- linux.vanilla-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c	2007-09-26 16:46:48.000000000 +0100
> +++ linux-2.6.23rc8-mm1/drivers/ata/pata_hpt37x.c	2007-09-18 16:44:32.000000000 +0100

    Wait, I thought you're patching pata_hpt3x2n!

> @@ -844,6 +844,46 @@
>  	/* Never went stable */
>  	return 0;
>  }
> +
> +static void *hpt_tune_function(struct pci_dev *dev, int dpll, int clock_slot)
> +{
> +	static const int MHz[4] = { 33, 40, 50, 66 };
> +	/*
> +	 *	For non UDMA133 capable devices we should
> +	 *	use a 50MHz DPLL by choice
> +	 */
> +	unsigned int f_low, f_high;
> +	int adjust;
> +
> +	f_low = (MHz[clock_slot] * 48) / MHz[dpll];
> +	f_high = f_low + 2;
> +	if (clock_slot > 1)
> +		f_high += 2;
> +	/* Select the DPLL clock. */
> +	pci_write_config_byte(dev, 0x5b, 0x21);
> +	pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);

    Could move the f_low/high writes to hpt37x_calibrate_dpll()...

> +	for(adjust = 0; adjust < 8; adjust++) {
> +		if (hpt37x_calibrate_dpll(dev))
> +			break;
> +		/* See if it'll settle at a fractionally different clock */
> +		if (adjust & 1)
> +			f_low -= adjust >> 1;
> +		else
> +			f_high += adjust >> 1;
> +		pci_write_config_dword(dev, 0x5C, (f_high << 16) | f_low | 0x100);
> +	}
> +	if (adjust == 8) {
> +		printk(KERN_WARNING "hpt37x: DPLL did not stabilize.\n");

    Deserves KERN_ERR. Wait, I remember to have changed it to KERN_ERR.

> +		return NULL;
> +	}
> +	printk(KERN_INFO "hpt37x: Bus clock %dMHz, using DPLL.\n", MHz[dpll]);

    That won't do -- it reintroduces the DPLL clock ISO bus clock reporting 
bug (that I've fixed just recently)!

> @@ -944,7 +984,7 @@
>  	u8 mcr1;
>  	u32 freq;
>  	int prefer_dpll = 1;
> -
> +	int hpt374alt = 0;
>  	unsigned long iobase = pci_resource_start(dev, 4);
>  
>  	const struct hpt_chip *chip_table;
> @@ -1046,16 +1086,28 @@
>  	if (chip_table == &hpt372a)
>  		outb(0x0e, iobase + 0x9c);
>  
> +	/*
> +	 * PLL must be done once
> +	 */
> +	 
> +	if (chip_table == &hpt374 && PCI_FUNC(dev->devfn) & 1) {
> +		/* The HPT374 secondary devfn is tuned to 50MHz when we find
> +		   the primary */

    Nah, the reason for that is completely different -- BIOS saves no clock 
data for function 1 but calibrates it for 50 MHz anyway, thus making the 
driver read already distorted f_CNT...
Not actually dangerous as the value yields 33 MHz PCI clock after clamping but 
WTF...

> +		port_info = *port;
> +		port_info.private_data = hpt37x_timings_50;
> +	}

    No, this does not seem correct -- the function 1 should be tuned (for the 
x86 case where there was no BIOS available to tune it beforehand). I don't see 
where the manual tells that the DPLL circuitry is shared. Contrarywise, I'm 
seeing it say on p. 3-22:

2. Each function has its own DPLL module, so you need set DPLL clock on every
    function

>  	/* Some devices do not let this value be accessed via PCI space
>  	   according to the old driver */
> -
>  	freq = inl(iobase + 0x90);
>  	if ((freq >> 12) != 0xABCDE) {
>  		int i;
>  		u8 sr;
>  		u32 total = 0;
> -
> +		
>  		printk(KERN_WARNING "pata_hpt37x: BIOS has not set timing clocks.\n");
> +		if (hpt374alt == 1)
> +			printk(KERN_ERR "pata_hpt37x: No saved frequency on primary function.\n");

    What's so erratic about this? And where hpt374alt is set to 1?

> +		private_data = hpt_tune_function(dev, dpll, clock_slot);
> +		/* For the HPT374 tune both channels together from fn 0 */
> +		if (chip_table == &hpt374 && !(PCI_FUNC(dev->devfn) & 1)) {
> +			struct pci_dev *pair = pci_get_slot(dev->bus, dev->devfn + 1);
> +			if (pair != NULL) {
> +				hpt_tune_function(pair, dpll, clock_slot);
> +				pci_dev_put(pair);
> +			}

    Ah, I see it now: you've decided to tune 2 functions of HPT374 at once...

>  		}
> -		if (dpll == 3)
> -			private_data = (void *)hpt37x_timings_66;
> -		else
> -			private_data = (void *)hpt37x_timings_50;
> -
> -		printk(KERN_INFO "pata_hpt37x: bus clock %dMHz, using %dMHz DPLL.\n",
> -		       MHz[clock_slot], MHz[dpll]);

    There was no need to undo that.

>  	} else {
>  		private_data = (void *)chip_table->clocks[clock_slot];
>  		/*

MBR, Sergei

Re: [PATCH] pata_hpt3x2n: Clean up DPLL stuff

[Alan Cox]

On Mon, 01 Oct 2007 17:12:03 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Hello.
> 
> Alan Cox wrote:
> 
> > Nobody commented when I asked for review earlier so it must be ok  8)
> 
>     It's not that I've seen this before
> 
>  > so it must be ok  8)
> 
>     Not by me at least, so let me NAK it. 8-)
> 
> > Lets stick it in -mm to be sure
> 
>     Now let's unstick it. :-)

Ok I shall revisit that. Andrew please drop

Re: [PATCH] pata_hpt3x2n: Clean up DPLL stuff

[Jeff Garzik]

Alan Cox wrote:
> On Mon, 01 Oct 2007 17:12:03 +0400
> Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
>> Hello.
>>
>> Alan Cox wrote:
>>
>>> Nobody commented when I asked for review earlier so it must be ok  8)
>>     It's not that I've seen this before
>>
>>  > so it must be ok  8)
>>
>>     Not by me at least, so let me NAK it. 8-)
>>
>>> Lets stick it in -mm to be sure
>>     Now let's unstick it. :-)
> 
> Ok I shall revisit that. Andrew please drop

As I noted at the time of posting, this is in libata-dev.git#for-testing 
(and thus propagated to -mm via that route).  Consider it dropped, from 
my end.

	Jeff