pata_pdc2027x: PLL detection fixes

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

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

Quoted 1 line

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 <[email protected]>

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

drivers/ata/pata_pdc2027x.c +2 -2

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

Quoted 34 lines · Mikael Pettersson

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

Quoted 1 line

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.

Quoted 5 lines

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 <[email protected]>

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]

Quoted 25 lines

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

drivers/ata/pata_pdc2027x.c +4 -4

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

drivers/ata/pata_pdc2027x.c static long pdc_read_counter(struct ata_ · line 575

Quoted 1 line

drivers/ata/pata_pdc2027x.c +0 -0

     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.

Quoted 2 lines · Albert Lee

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.

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 <[email protected]>

---
Tested ok on both x86 and ppc64, together with Mikael's patch.

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

Quoted 1 line

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 <[email protected]>

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

drivers/ata/pata_pdc2027x.c +0 -2

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

drivers/ata/pata_pdc2027x.c +0 -4

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

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

Quoted 1 line

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 <[email protected]>

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

drivers/ata/pata_pdc2027x.c +0 -0

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

Quoted 1 line

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 <[email protected]>

---
 drivers/ata/pata_pdc2027x.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)