linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
@ 2007-08-18 20:58 Mikael Pettersson
  2007-08-18 21:25 ` Jeff Garzik
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Mikael Pettersson @ 2007-08-18 20:58 UTC (permalink / raw)
  To: linux-ide; +Cc: alan, albertl, jeff

Previously I reported that the pata_pdc2027x PLL detection changes
in kernel 2.6.22 broke the driver on my PowerMac:

>pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!

This is followed by a number of errors and speed reduction
steps on the affected ports.

There are two bugs in pata_pdc2027x's PLL detection code:

1. The PLL counter's start value is read before the chip is
   put in "test mode". Outside of test mode the counter is
   halted, and on the PowerMac the counter is zero because
   the chip hasn't been initialised by its BIOS.

   The fix is to move the read of the start value to after
   test mode is started, but before the mdelay() in test mode.
   This also improves the precision of the PLL detection.

2. The code to compute the number of PLL decrements during the
   mdelay() in test mode fails to consider that the PLL counter
   only is 30 bits wide. If there is a wraparound, it will compute
   an incorrect and much too large value. On the PowerMac, the
   start count is zero, the end count is a large 30-bit value, so
   wraparound occurs and an out of bounds PLL clock is detected.

   The fix is to mask the (start - end) computation to 30 bits.

While debugging this I also noticed that pdc_read_counter()
reads the two halves of the 30-bit PLL counter as 16-bit values,
and then combines them as if the halves only are 15 bits wide.
To avoid confusion, the halves should be read as 15-bit values.

This patch implements all three changes. It fixes the PLL detection
failure on my PowerMac, and doesn't cause any regressions on an x86
with an identical card.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
---
 drivers/ata/pata_pdc2027x.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff -rupN linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c
--- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c	2007-07-09 22:01:31.000000000 +0200
+++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c	2007-08-18 21:53:40.000000000 +0200
@@ -563,13 +563,13 @@ static long pdc_read_counter(struct ata_
 	u32 bccrl, bccrh, bccrlv, bccrhv;
 
 retry:
-	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
-	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
+	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
+	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
 	rmb();
 
 	/* Read the counter values again for verification */
-	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
-	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
+	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
+	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
 	rmb();
 
 	counter = (bccrh << 15) | bccrl;
@@ -692,16 +692,16 @@ static long pdc_detect_pll_input_clock(s
 	struct timeval start_time, end_time;
 	long pll_clock, usec_elapsed;
 
-	/* Read current counter value */
-	start_count = pdc_read_counter(host);
-	do_gettimeofday(&start_time);
-
 	/* Start the test mode */
 	scr = readl(mmio_base + PDC_SYS_CTL);
 	PDPRINTK("scr[%X]\n", scr);
 	writel(scr | (0x01 << 14), mmio_base + PDC_SYS_CTL);
 	readl(mmio_base + PDC_SYS_CTL); /* flush */
 
+	/* Read current counter value */
+	start_count = pdc_read_counter(host);
+	do_gettimeofday(&start_time);
+
 	/* Let the counter run for 100 ms. */
 	mdelay(100);
 
@@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s
 	usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 1000000 +
 		(end_time.tv_usec - start_time.tv_usec);
 
-	pll_clock = (start_count - end_count) / 100 *
+	pll_clock = ((start_count - end_count) & 0x3fffffff) / 100 *
 		(100000000 / usec_elapsed);
 
 	PDPRINTK("start[%ld] end[%ld] \n", start_count, end_count);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-18 20:58 [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Mikael Pettersson
@ 2007-08-18 21:25 ` Jeff Garzik
  2007-08-19  0:14   ` Albert Lee
  2007-08-19  0:01 ` [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Albert Lee
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Jeff Garzik @ 2007-08-18 21:25 UTC (permalink / raw)
  To: Mikael Pettersson; +Cc: linux-ide, alan, albertl

Mikael Pettersson wrote:
> Previously I reported that the pata_pdc2027x PLL detection changes
> in kernel 2.6.22 broke the driver on my PowerMac:
> 
>> pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!
> 
> This is followed by a number of errors and speed reduction
> steps on the affected ports.
> 
> There are two bugs in pata_pdc2027x's PLL detection code:
> 
> 1. The PLL counter's start value is read before the chip is
>    put in "test mode". Outside of test mode the counter is
>    halted, and on the PowerMac the counter is zero because
>    the chip hasn't been initialised by its BIOS.
> 
>    The fix is to move the read of the start value to after
>    test mode is started, but before the mdelay() in test mode.
>    This also improves the precision of the PLL detection.
> 
> 2. The code to compute the number of PLL decrements during the
>    mdelay() in test mode fails to consider that the PLL counter
>    only is 30 bits wide. If there is a wraparound, it will compute
>    an incorrect and much too large value. On the PowerMac, the
>    start count is zero, the end count is a large 30-bit value, so
>    wraparound occurs and an out of bounds PLL clock is detected.
> 
>    The fix is to mask the (start - end) computation to 30 bits.
> 
> While debugging this I also noticed that pdc_read_counter()
> reads the two halves of the 30-bit PLL counter as 16-bit values,
> and then combines them as if the halves only are 15 bits wide.
> To avoid confusion, the halves should be read as 15-bit values.
> 
> This patch implements all three changes. It fixes the PLL detection
> failure on my PowerMac, and doesn't cause any regressions on an x86
> with an identical card.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

Fantastic!  Thanks for putting in a great effort to track these down.

I'll queue it up [unless someone responds with a problem requiring 
revision, of course]


> diff -rupN linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c
> --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c	2007-07-09 22:01:31.000000000 +0200
> +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c	2007-08-18 21:53:40.000000000 +0200
> @@ -563,13 +563,13 @@ static long pdc_read_counter(struct ata_
>  	u32 bccrl, bccrh, bccrlv, bccrhv;
>  
>  retry:
> -	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
> -	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
> +	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
> +	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>  	rmb();
>  
>  	/* Read the counter values again for verification */
> -	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
> -	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
> +	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
> +	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>  	rmb();
>  
>  	counter = (bccrh << 15) | bccrl;

Unrelated to your changes, but, I wonder why those rmb() are there at 
all...?

	Jeff




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-18 20:58 [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Mikael Pettersson
  2007-08-18 21:25 ` Jeff Garzik
@ 2007-08-19  0:01 ` Albert Lee
  2007-08-19 16:13 ` Sergei Shtylyov
  2007-08-23  9:32 ` Jeff Garzik
  3 siblings, 0 replies; 10+ messages in thread
From: Albert Lee @ 2007-08-19  0:01 UTC (permalink / raw)
  To: Mikael Pettersson; +Cc: linux-ide, alan, albertl, jeff

Mikael Pettersson wrote:
> Previously I reported that the pata_pdc2027x PLL detection changes
> in kernel 2.6.22 broke the driver on my PowerMac:
> 
> 
>>pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!
> 
> 
> This is followed by a number of errors and speed reduction
> steps on the affected ports.
> 
> There are two bugs in pata_pdc2027x's PLL detection code:
> 
> 1. The PLL counter's start value is read before the chip is
>    put in "test mode". Outside of test mode the counter is
>    halted, and on the PowerMac the counter is zero because
>    the chip hasn't been initialised by its BIOS.
> 
>    The fix is to move the read of the start value to after
>    test mode is started, but before the mdelay() in test mode.
>    This also improves the precision of the PLL detection.
> 
> 2. The code to compute the number of PLL decrements during the
>    mdelay() in test mode fails to consider that the PLL counter
>    only is 30 bits wide. If there is a wraparound, it will compute
>    an incorrect and much too large value. On the PowerMac, the
>    start count is zero, the end count is a large 30-bit value, so
>    wraparound occurs and an out of bounds PLL clock is detected.
> 
>    The fix is to mask the (start - end) computation to 30 bits.

The wrap-around situation was handled by checking if (pll_clock < 0)
in pdc_hardware_init() then retrying pdc_detect_pll_input_clock().
But it seems not working correctly. :(

> 
> While debugging this I also noticed that pdc_read_counter()
> reads the two halves of the 30-bit PLL counter as 16-bit values,
> and then combines them as if the halves only are 15 bits wide.
> To avoid confusion, the halves should be read as 15-bit values.
> 
> This patch implements all three changes. It fixes the PLL detection
> failure on my PowerMac, and doesn't cause any regressions on an x86
> with an identical card.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
>  drivers/ata/pata_pdc2027x.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff -rupN linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c
> --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c	2007-07-09 22:01:31.000000000 +0200
> +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c	2007-08-18 21:53:40.000000000 +0200
> @@ -563,13 +563,13 @@ static long pdc_read_counter(struct ata_
>  	u32 bccrl, bccrh, bccrlv, bccrhv;
>  
>  retry:
> -	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
> -	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
> +	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
> +	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>  	rmb();
>  
>  	/* Read the counter values again for verification */
> -	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
> -	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
> +	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
> +	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>  	rmb();

Agreed this looks safer. (Although the high bit 15 of the counter
is always zero during my test.)

>  
>  	counter = (bccrh << 15) | bccrl;
> @@ -692,16 +692,16 @@ static long pdc_detect_pll_input_clock(s
>  	struct timeval start_time, end_time;
>  	long pll_clock, usec_elapsed;
>  
> -	/* Read current counter value */
> -	start_count = pdc_read_counter(host);
> -	do_gettimeofday(&start_time);
> -
>  	/* Start the test mode */
>  	scr = readl(mmio_base + PDC_SYS_CTL);
>  	PDPRINTK("scr[%X]\n", scr);
>  	writel(scr | (0x01 << 14), mmio_base + PDC_SYS_CTL);
>  	readl(mmio_base + PDC_SYS_CTL); /* flush */
>  
> +	/* Read current counter value */
> +	start_count = pdc_read_counter(host);
> +	do_gettimeofday(&start_time);
> +
>  	/* Let the counter run for 100 ms. */
>  	mdelay(100);

Looks good (though reading the start_count before or after doesn't
really matter since we can treat "start the test mode" as part of
the 100ms delay time).

>  
> @@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s
>  	usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 1000000 +
>  		(end_time.tv_usec - start_time.tv_usec);
>  
> -	pll_clock = (start_count - end_count) / 100 *
> +	pll_clock = ((start_count - end_count) & 0x3fffffff) / 100 *
>  		(100000000 / usec_elapsed);
>  
>  	PDPRINTK("start[%ld] end[%ld] \n", start_count, end_count);
> 
> 

Hmm, this one alone looks like the real fix for the problem. I guess my
previous patch changing pll_clock from "(start_count - end_count) * 10"
to "(start_count - end_count) / 100 * (100000000 / usec_elapsed)" somehow
broke the (pll_clock < 0) check...

Thanks for fixing it. Maybe we also need this fix for the IDE
pdc202xx_new.c driver...

Acked-by: Albert Lee <albertcc@tw.ibm.com>
--
albert


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-18 21:25 ` Jeff Garzik
@ 2007-08-19  0:14   ` Albert Lee
  2007-08-19  0:53     ` Jeff Garzik
  0 siblings, 1 reply; 10+ messages in thread
From: Albert Lee @ 2007-08-19  0:14 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Mikael Pettersson, linux-ide, alan, albertl

Jeff Garzik wrote:
> Mikael Pettersson wrote:
> 
>> Previously I reported that the pata_pdc2027x PLL detection changes
>> in kernel 2.6.22 broke the driver on my PowerMac:
>>
>>> pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!
>>
>>
>> This is followed by a number of errors and speed reduction
>> steps on the affected ports.
>>
>> There are two bugs in pata_pdc2027x's PLL detection code:
>>
>> 1. The PLL counter's start value is read before the chip is
>>    put in "test mode". Outside of test mode the counter is
>>    halted, and on the PowerMac the counter is zero because
>>    the chip hasn't been initialised by its BIOS.
>>
>>    The fix is to move the read of the start value to after
>>    test mode is started, but before the mdelay() in test mode.
>>    This also improves the precision of the PLL detection.
>>
>> 2. The code to compute the number of PLL decrements during the
>>    mdelay() in test mode fails to consider that the PLL counter
>>    only is 30 bits wide. If there is a wraparound, it will compute
>>    an incorrect and much too large value. On the PowerMac, the
>>    start count is zero, the end count is a large 30-bit value, so
>>    wraparound occurs and an out of bounds PLL clock is detected.
>>
>>    The fix is to mask the (start - end) computation to 30 bits.
>>
>> While debugging this I also noticed that pdc_read_counter()
>> reads the two halves of the 30-bit PLL counter as 16-bit values,
>> and then combines them as if the halves only are 15 bits wide.
>> To avoid confusion, the halves should be read as 15-bit values.
>>
>> This patch implements all three changes. It fixes the PLL detection
>> failure on my PowerMac, and doesn't cause any regressions on an x86
>> with an identical card.
>>
>> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> 
> 
> Fantastic!  Thanks for putting in a great effort to track these down.
> 
> I'll queue it up [unless someone responds with a problem requiring
> revision, of course]
> 
> 
>> diff -rupN linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c
>> linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c
>>
>> --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c    2007-07-09
>> 22:01:31.000000000 +0200
>> +++
>> linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c   
>> 2007-08-18 21:53:40.000000000 +0200
>> @@ -563,13 +563,13 @@ static long pdc_read_counter(struct ata_
>>      u32 bccrl, bccrh, bccrlv, bccrhv;
>>  
>>  retry:
>> -    bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
>> -    bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
>> +    bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
>> +    bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>>      rmb();
>>  
>>      /* Read the counter values again for verification */
>> -    bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0xffff;
>> -    bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0xffff;
>> +    bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
>> +    bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
>>      rmb();
>>  
>>      counter = (bccrh << 15) | bccrl;
> 
> 
> Unrelated to your changes, but, I wonder why those rmb() are there at
> all...?
> 

The first rmb() is to make sure bccrl is read before bccrlv for later
(bccrl >= bccrlv) check since both reading the same memory address.

The second rmb() looks like leftover when the code copy-and-paste'ed.
Maybe we can remove this one.
--
albert



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-19  0:14   ` Albert Lee
@ 2007-08-19  0:53     ` Jeff Garzik
  2007-08-19  1:03       ` Albert Lee
  2007-08-20  8:56       ` [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup Albert Lee
  0 siblings, 2 replies; 10+ messages in thread
From: Jeff Garzik @ 2007-08-19  0:53 UTC (permalink / raw)
  To: albertl; +Cc: Mikael Pettersson, linux-ide, alan

Albert Lee wrote:
> The first rmb() is to make sure bccrl is read before bccrlv for later
> (bccrl >= bccrlv) check since both reading the same memory address.

That's already guaranteed without the rmb(), AFAICS.

	Jeff



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-19  0:53     ` Jeff Garzik
@ 2007-08-19  1:03       ` Albert Lee
  2007-08-20  8:56       ` [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup Albert Lee
  1 sibling, 0 replies; 10+ messages in thread
From: Albert Lee @ 2007-08-19  1:03 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: albertl, Mikael Pettersson, linux-ide, alan

Jeff Garzik wrote:
> Albert Lee wrote:
> 
>> The first rmb() is to make sure bccrl is read before bccrlv for later
>> (bccrl >= bccrlv) check since both reading the same memory address.
> 
> 
> That's already guaranteed without the rmb(), AFAICS.
> 

Hmm, thanks for the advice. Will remove both rmb()s.
Will also remove the now unused (pll_clock < 0) check from pdc_hardware_init()
and test/verify Mikael's patch when I go back to office tomorrow.

--
albert


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-18 20:58 [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Mikael Pettersson
  2007-08-18 21:25 ` Jeff Garzik
  2007-08-19  0:01 ` [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Albert Lee
@ 2007-08-19 16:13 ` Sergei Shtylyov
  2007-08-23  9:32 ` Jeff Garzik
  3 siblings, 0 replies; 10+ messages in thread
From: Sergei Shtylyov @ 2007-08-19 16:13 UTC (permalink / raw)
  To: Mikael Pettersson; +Cc: linux-ide, alan, albertl, jeff

Mikael Pettersson wrote:

> Previously I reported that the pata_pdc2027x PLL detection changes
> in kernel 2.6.22 broke the driver on my PowerMac:

>>pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!

> This is followed by a number of errors and speed reduction
> steps on the affected ports.

> There are two bugs in pata_pdc2027x's PLL detection code:

> 1. The PLL counter's start value is read before the chip is
>    put in "test mode". Outside of test mode the counter is
>    halted, and on the PowerMac the counter is zero because
>    the chip hasn't been initialised by its BIOS.

    So what?

>    The fix is to move the read of the start value to after
>    test mode is started, but before the mdelay() in test mode.

    This is not an issue, so no fix is needed.

>    This also improves the precision of the PLL detection.

    BTW, looks like we don't even need to bother reading the darn counter 
beforehand: bit 1 of the indexed register 1 (the same used to enter/exit test 
mode by twiddling its bit 6) when being cleared should reset the counter to 0 
-- I'm looking at the internal sources which were written based on the 
*fragment* of the PDC20270 datasheet (yeah, Promise didn't even give us the 
whole datasheet!) about the PLL calibration.

> 2. The code to compute the number of PLL decrements during the
>    mdelay() in test mode fails to consider that the PLL counter
>    only is 30 bits wide. If there is a wraparound, it will compute
>    an incorrect and much too large value. On the PowerMac, the
>    start count is zero, the end count is a large 30-bit value, so
>    wraparound occurs and an out of bounds PLL clock is detected.

>    The fix is to mask the (start - end) computation to 30 bits.

    Yeah, that's what I've done for the old IDE driver...

> While debugging this I also noticed that pdc_read_counter()
> reads the two halves of the 30-bit PLL counter as 16-bit values,
> and then combines them as if the halves only are 15 bits wide.
> To avoid confusion, the halves should be read as 15-bit values.

    Shouldn't matter, the bit is most probably reseved and so always remains 0.
Actually, those 2 counters count the data bytes transferred over PCI bus when 
the chip in not in the test mode.

> This patch implements all three changes. It fixes the PLL detection
> failure on my PowerMac, and doesn't cause any regressions on an x86
> with an identical card.

> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

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

> diff -rupN linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c
> --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c	2007-07-09 22:01:31.000000000 +0200
> +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c	2007-08-18 21:53:40.000000000 +0200
[...]
> @@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s
>  	usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 1000000 +
>  		(end_time.tv_usec - start_time.tv_usec);
>  
> -	pll_clock = (start_count - end_count) / 100 *
> +	pll_clock = ((start_count - end_count) & 0x3fffffff) / 100 *
>  		(100000000 / usec_elapsed);
>  
>  	PDPRINTK("start[%ld] end[%ld] \n", start_count, end_count);

    Only this fragment really matters.

MBR, Sergei

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup
  2007-08-19  0:53     ` Jeff Garzik
  2007-08-19  1:03       ` Albert Lee
@ 2007-08-20  8:56       ` Albert Lee
  2007-08-31  9:35         ` Jeff Garzik
  1 sibling, 1 reply; 10+ messages in thread
From: Albert Lee @ 2007-08-20  8:56 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Mikael Pettersson, linux-ide, alan, Sergei Shtylyov


Minor cleanup to remove the unneeded rmb()s per Jeff's advice. Also removed the
pll_clock < 0 check since pll_clock now guaranteed to be >= 0 after Mikael's patch.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Tested ok on both x86 and ppc64, together with Mikael's patch.

diff -Nrup 01_mikael/drivers/ata/pata_pdc2027x.c 02_pdc_pll_fix2/drivers/ata/pata_pdc2027x.c
--- 01_mikael/drivers/ata/pata_pdc2027x.c	2007-08-20 10:43:38.000000000 +0800
+++ 02_pdc_pll_fix2/drivers/ata/pata_pdc2027x.c	2007-08-20 10:48:22.000000000 +0800
@@ -565,12 +565,10 @@ static long pdc_read_counter(struct ata_
 retry:
 	bccrl = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
 	bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
-	rmb();
 
 	/* Read the counter values again for verification */
 	bccrlv = readl(mmio_base + PDC_BYTE_COUNT) & 0x7fff;
 	bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) & 0x7fff;
-	rmb();
 
 	counter = (bccrh << 15) | bccrl;
 
@@ -745,9 +743,6 @@ static int pdc_hardware_init(struct ata_
 	 */
 	pll_clock = pdc_detect_pll_input_clock(host);
 
-	if (pll_clock < 0) /* counter overflow? Try again. */
-		pll_clock = pdc_detect_pll_input_clock(host);
-
 	dev_printk(KERN_INFO, host->dev, "PLL input clock %ld kHz\n", pll_clock/1000);
 
 	/* Adjust PLL control register */



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
  2007-08-18 20:58 [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Mikael Pettersson
                   ` (2 preceding siblings ...)
  2007-08-19 16:13 ` Sergei Shtylyov
@ 2007-08-23  9:32 ` Jeff Garzik
  3 siblings, 0 replies; 10+ messages in thread
From: Jeff Garzik @ 2007-08-23  9:32 UTC (permalink / raw)
  To: Mikael Pettersson; +Cc: linux-ide, alan, albertl

Mikael Pettersson wrote:
> Previously I reported that the pata_pdc2027x PLL detection changes
> in kernel 2.6.22 broke the driver on my PowerMac:
> 
>> pata_pdc2027x: Invalid PLL input clock 1691742kHz, give up!
> 
> This is followed by a number of errors and speed reduction
> steps on the affected ports.
> 
> There are two bugs in pata_pdc2027x's PLL detection code:
> 
> 1. The PLL counter's start value is read before the chip is
>    put in "test mode". Outside of test mode the counter is
>    halted, and on the PowerMac the counter is zero because
>    the chip hasn't been initialised by its BIOS.
> 
>    The fix is to move the read of the start value to after
>    test mode is started, but before the mdelay() in test mode.
>    This also improves the precision of the PLL detection.
> 
> 2. The code to compute the number of PLL decrements during the
>    mdelay() in test mode fails to consider that the PLL counter
>    only is 30 bits wide. If there is a wraparound, it will compute
>    an incorrect and much too large value. On the PowerMac, the
>    start count is zero, the end count is a large 30-bit value, so
>    wraparound occurs and an out of bounds PLL clock is detected.
> 
>    The fix is to mask the (start - end) computation to 30 bits.
> 
> While debugging this I also noticed that pdc_read_counter()
> reads the two halves of the 30-bit PLL counter as 16-bit values,
> and then combines them as if the halves only are 15 bits wide.
> To avoid confusion, the halves should be read as 15-bit values.
> 
> This patch implements all three changes. It fixes the PLL detection
> failure on my PowerMac, and doesn't cause any regressions on an x86
> with an identical card.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
>  drivers/ata/pata_pdc2027x.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)

applied



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup
  2007-08-20  8:56       ` [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup Albert Lee
@ 2007-08-31  9:35         ` Jeff Garzik
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff Garzik @ 2007-08-31  9:35 UTC (permalink / raw)
  To: albertl; +Cc: Mikael Pettersson, linux-ide, alan, Sergei Shtylyov

Albert Lee wrote:
> Minor cleanup to remove the unneeded rmb()s per Jeff's advice. Also removed the
> pll_clock < 0 check since pll_clock now guaranteed to be >= 0 after Mikael's patch.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> Tested ok on both x86 and ppc64, together with Mikael's patch.

applied to #upstream



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2007-08-31  9:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-18 20:58 [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Mikael Pettersson
2007-08-18 21:25 ` Jeff Garzik
2007-08-19  0:14   ` Albert Lee
2007-08-19  0:53     ` Jeff Garzik
2007-08-19  1:03       ` Albert Lee
2007-08-20  8:56       ` [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup Albert Lee
2007-08-31  9:35         ` Jeff Garzik
2007-08-19  0:01 ` [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes Albert Lee
2007-08-19 16:13 ` Sergei Shtylyov
2007-08-23  9:32 ` Jeff Garzik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).