[PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Mikael Pettersson]

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);

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Jeff Garzik]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Albert Lee]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Jeff Garzik]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Albert Lee]

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

[PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup

[Albert Lee]

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 */

Re: [PATCH 2.6.23-rc3] libata: pata_pdc2027x PLL detection minor cleanup

[Jeff Garzik]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Albert Lee]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Sergei Shtylyov]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Jeff Garzik]

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

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Mikael Pettersson]

On Sun, 19 Aug 2007 20:13:46 +0400, Sergei Shtylyov 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.
> 
>     So what?

a) causes an unnecessary wraparound, which in turn is one of the
   causes for PLL detection failures on non-x86
b) puts more work [the enter test mode stuff] in between the start
   and and sampling points, reducing the precision of the PLL
   detection; I actually observed quite noticeable differences
   in detected PLL frequency based on whether the start was sampled
   before or after the test mode enter code

> >    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.

Well, I have no data sheet and no sources except what's in the kernel
and what debug info PDC_DEBUG generates.

> > 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...

Except that due to what may be a typo pdc202xx_new masks to
26 bits, not 30. I was going to address that if/when this patch
goes in.

> > 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.

It matters when someone reads the code and wonders why two 16-bit values
(readl() & 0xffff) are combined with a 15-bit shift ((x << 15) | y).

> > 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.

Yeah, this is the key fix. But the other changes still improve things.

/Mikael

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Sergei Shtylyov]

Hello.

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?

> a) causes an unnecessary wraparound, which in turn is one of the
>    causes for PLL detection failures on non-x86

    It is *not* the cause of failure -- the old IDE driver copes well with 
this on non-x86. ;-)

> b) puts more work [the enter test mode stuff] in between the start
>    and and sampling points, reducing the precision of the PLL
>    detection; I actually observed quite noticeable differences
>    in detected PLL frequency based on whether the start was sampled
>    before or after the test mode enter code

    I'd think this differnce is negligible with 100 ms delay. But why not -- 
shouldn't harm indeed (except it's better to read a stable counter before than 
unstable one after entering test mode)?

>>>   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 

    Or maybe to 0x7fff?  I can't remember already -- never seen those infamous 
Promise papers, and I was testing this code looong ago already... :-)

>>-- 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.

> Well, I have no data sheet and no sources except what's in the kernel
> and what debug info PDC_DEBUG generates.

    IIRC, Albert should have the docs but he's under NDA.
    What have really surpried me about Promise was that they gave their SATA 
chip docs to Jeff who made them public and yet they continue to conceal the 
old PATA chip docs... :-/

>>>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...

> Except that due to what may be a typo pdc202xx_new masks to
> 26 bits, not 30.

    Indeed! :-<
    Thanks for noticing -- this is a typo, of course... And it's a pity that 
Albert failed to notice it when he last touched that driver...

> I was going to address that if/when this patch
> goes in.

    Please do, I'm too short of time.  But I guess the difference was never 
that large, so the clipped mask worked...
    I wonder if the true reason of the former issue which was attibuted 
mdelay() being imprecise...

>>>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.

> It matters when someone reads the code and wonders why two 16-bit values
> (readl() & 0xffff) are combined with a 15-bit shift ((x << 15) | y).

    Well, let it be. :-)

>>>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.

> Yeah, this is the key fix. But the other changes still improve things.

    Well, I've already given you my vote. :-)

> /Mikael

WBR, Sergei

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Jeff Garzik]

Sergei Shtylyov wrote:
>    What have really surpried me about Promise was that they gave their 
> SATA chip docs to Jeff who made them public and yet they continue to 
> conceal the old PATA chip docs... :-/


They probably just need to be poked.  I've been sitting on the sata_sx4 
docs and cards, waiting on them to give me permission to post the SX4 
docs since January.  Previous doc pokings were successful, but sometimes 
they disappear for months at a time.

	Jeff

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>>    What have really surpried me about Promise was that they gave their 
>> SATA chip docs to Jeff who made them public and yet they continue to 
>> conceal the old PATA chip docs... :-/

> They probably just need to be poked.  I've been sitting on the sata_sx4 

    It's not tat MV not poked them but al we've got was a hard copy of one 
section under an agreement to ship the calibration code only to the 
customer... :-/
     Luckily, there were Albert's patches floating around by rhis time. :-)

> docs and cards, waiting on them to give me permission to post the SX4 
> docs since January.  Previous doc pokings were successful, but sometimes 
> they disappear for months at a time.

>     Jeff

MBR, Sergei

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Albert Lee]

Sergei Shtylyov wrote:
> Hello.
> 
> 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?
> 
> 
>> a) causes an unnecessary wraparound, which in turn is one of the
>>    causes for PLL detection failures on non-x86
> 
> 
>    It is *not* the cause of failure -- the old IDE driver copes well
> with this on non-x86. ;-)
> 
>> b) puts more work [the enter test mode stuff] in between the start
>>    and and sampling points, reducing the precision of the PLL
>>    detection; I actually observed quite noticeable differences
>>    in detected PLL frequency based on whether the start was sampled
>>    before or after the test mode enter code
> 
> 
>    I'd think this differnce is negligible with 100 ms delay. But why not
> -- shouldn't harm indeed (except it's better to read a stable counter
> before than unstable one after entering test mode)?
> 
>>>>   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 
> 
> 
>    Or maybe to 0x7fff?  I can't remember already -- never seen those
> infamous Promise papers, and I was testing this code looong ago
> already... :-)


I've tested reloading the pata_pdc2027x module before.
The counter seems not cleared when leaving the test mode.


> 
>>> -- 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.
> 
> 
>> Well, I have no data sheet and no sources except what's in the kernel
>> and what debug info PDC_DEBUG generates.
> 
> 
>    IIRC, Albert should have the docs but he's under NDA.
>    What have really surpried me about Promise was that they gave their
> SATA chip docs to Jeff who made them public and yet they continue to
> conceal the old PATA chip docs... :-/
> 
>>>> 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...
> 
> 
>> Except that due to what may be a typo pdc202xx_new masks to
>> 26 bits, not 30.
> 
> 
>    Indeed! :-<
>    Thanks for noticing -- this is a typo, of course... And it's a pity
> that Albert failed to notice it when he last touched that driver...

I was too blind to notice the wrong 26-bit mask. :(

Fortunately with the 10ms delay used by the IDE pdc202xx_new driver,
(start_count - end_count) is smaller than the 26-bit mask. So it
doesn't actually damage anything.

> 
>> I was going to address that if/when this patch
>> goes in.
> 
> 
>    Please do, I'm too short of time.  But I guess the difference was
> never that large, so the clipped mask worked...
>    I wonder if the true reason of the former issue which was attibuted
> mdelay() being imprecise...

mdelay() is actually imprecise on my x86 PC. This can be measured by
doing some do_gettimeofday() tests. It's quite precise when tested on
ppc64...

--
albert

> 
>>>> 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.
> 
> 
>> It matters when someone reads the code and wonders why two 16-bit values
>> (readl() & 0xffff) are combined with a 15-bit shift ((x << 15) | y).
> 
> 
>    Well, let it be. :-)
> 
>>>> 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>
> 
> 

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Sergei Shtylyov]

Hello.

Albert Lee wrote:

>>>b) puts more work [the enter test mode stuff] in between the start
>>>   and and sampling points, reducing the precision of the PLL
>>>   detection; I actually observed quite noticeable differences
>>>   in detected PLL frequency based on whether the start was sampled
>>>   before or after the test mode enter code

>>   I'd think this differnce is negligible with 100 ms delay. But why not
>>-- shouldn't harm indeed (except it's better to read a stable counter
>>before than unstable one after entering test mode)?

>>>>>  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 

>>   Or maybe to 0x7fff?  I can't remember already -- never seen those
>>infamous Promise papers, and I was testing this code looong ago
>>already... :-)

> I've tested reloading the pata_pdc2027x module before.
> The counter seems not cleared when leaving the test mode.

     I never saud that. Counter should be cleared by resetting "test mode" bit.
But I'm seeing the cause of misinteprettion: one may think that bit 1 is used 
to clear counters but I meant to say that clearing bit 1 of that ssme register 
should clear the counters...

>>>>>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...

>>>Except that due to what may be a typo pdc202xx_new masks to
>>>26 bits, not 30.

>>   Indeed! :-<
>>   Thanks for noticing -- this is a typo, of course... And it's a pity
>>that Albert failed to notice it when he last touched that driver...

> I was too blind to notice the wrong 26-bit mask. :(

> Fortunately with the 10ms delay used by the IDE pdc202xx_new driver,
> (start_count - end_count) is smaller than the 26-bit mask. So it
> doesn't actually damage anything.

    Yeah, I thought so.

>>>I was going to address that if/when this patch
>>>goes in.

>>   Please do, I'm too short of time.  But I guess the difference was
>>never that large, so the clipped mask worked...
>>   I wonder if the true reason of the former issue which was attibuted
>>mdelay() being imprecise...

> mdelay() is actually imprecise on my x86 PC. This can be measured by
> doing some do_gettimeofday() tests. It's quite precise when tested on
> ppc64...

    Hm...

> --
> albert

    Don't overquote, please. ;-)

MBR, Sergei

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Bartlomiej Zolnierkiewicz]

On Friday 24 August 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Albert Lee wrote:
> 
> >>>b) puts more work [the enter test mode stuff] in between the start
> >>>   and and sampling points, reducing the precision of the PLL
> >>>   detection; I actually observed quite noticeable differences
> >>>   in detected PLL frequency based on whether the start was sampled
> >>>   before or after the test mode enter code
> 
> >>   I'd think this differnce is negligible with 100 ms delay. But why not
> >>-- shouldn't harm indeed (except it's better to read a stable counter
> >>before than unstable one after entering test mode)?
> 
> >>>>>  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 
> 
> >>   Or maybe to 0x7fff?  I can't remember already -- never seen those
> >>infamous Promise papers, and I was testing this code looong ago
> >>already... :-)
> 
> > I've tested reloading the pata_pdc2027x module before.
> > The counter seems not cleared when leaving the test mode.
> 
>      I never saud that. Counter should be cleared by resetting "test mode" bit.
> But I'm seeing the cause of misinteprettion: one may think that bit 1 is used 
> to clear counters but I meant to say that clearing bit 1 of that ssme register 
> should clear the counters...
> 
> >>>>>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...
> 
> >>>Except that due to what may be a typo pdc202xx_new masks to
> >>>26 bits, not 30.
> 
> >>   Indeed! :-<
> >>   Thanks for noticing -- this is a typo, of course... And it's a pity
> >>that Albert failed to notice it when he last touched that driver...
> 
> > I was too blind to notice the wrong 26-bit mask. :(
> 
> > Fortunately with the 10ms delay used by the IDE pdc202xx_new driver,
> > (start_count - end_count) is smaller than the 26-bit mask. So it
> > doesn't actually damage anything.
> 
>     Yeah, I thought so.

I'm a bit lost in this discussion.

Do we need some of these pata_pdc2027x fixes in pdc202xx_new or not?

Thanks,
Bart

Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>>b) puts more work [the enter test mode stuff] in between the start
>>>>>  and and sampling points, reducing the precision of the PLL
>>>>>  detection; I actually observed quite noticeable differences
>>>>>  in detected PLL frequency based on whether the start was sampled
>>>>>  before or after the test mode enter code

>>>>  I'd think this differnce is negligible with 100 ms delay. But why not
>>>>-- shouldn't harm indeed (except it's better to read a stable counter
>>>>before than unstable one after entering test mode)?

>>>>>>> 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 

>>>>  Or maybe to 0x7fff?  I can't remember already -- never seen those
>>>>infamous Promise papers, and I was testing this code looong ago
>>>>already... :-)

>>>I've tested reloading the pata_pdc2027x module before.
>>>The counter seems not cleared when leaving the test mode.

>>     I never saud that. Counter should be cleared by resetting "test mode" bit.

     I meant to say "shouldn't".

>>But I'm seeing the cause of misinteprettion: one may think that bit 1 is used 
>>to clear counters but I meant to say that clearing bit 1 of that ssme register 
>>should clear the counters...

>>>>>>>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...

>>>>>Except that due to what may be a typo pdc202xx_new masks to
>>>>>26 bits, not 30.

>>>>  Indeed! :-<
>>>>  Thanks for noticing -- this is a typo, of course... And it's a pity
>>>>that Albert failed to notice it when he last touched that driver...

>>>I was too blind to notice the wrong 26-bit mask. :(

>>>Fortunately with the 10ms delay used by the IDE pdc202xx_new driver,
>>>(start_count - end_count) is smaller than the 26-bit mask. So it
>>>doesn't actually damage anything.

>>    Yeah, I thought so.

> I'm a bit lost in this discussion.

> Do we need some of these pata_pdc2027x fixes in pdc202xx_new or not?

    Mikael alredy has a patch fixing the clipped mask for end_time and 
start_time difference -- 0x3fffffff ISO of 0x3ffffff.

> Thanks,
> Bart

MBR, Sergei