Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 -- 0x3fff ISO of 0x3ff. Thanks, Bart MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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() 0x) 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 [EMAIL PROTECTED] - To unsubscribe from
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 [EMAIL PROTECTED] Acked-by: Sergei Shtylyov [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 --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +0200 [...] @@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 100 + (end_time.tv_usec - start_time.tv_usec); - pll_clock = (start_count - end_count) / 100 * + pll_clock = ((start_count - end_count) 0x3fff) / 100 * (1 / usec_elapsed); PDPRINTK(start[%ld] end[%ld] \n, start_count, end_count); Only this fragment really matters. MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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() 0x) 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 [EMAIL PROTECTED] Acked-by: Sergei Shtylyov [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 --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +0200 [...] @@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 100 + (end_time.tv_usec - start_time.tv_usec); - pll_clock = (start_count - end_count) / 100 * + pll_clock = ((start_count - end_count) 0x3fff) / 100 * (1 / 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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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() 0x) 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 [EMAIL PROTECTED] Acked-by: Sergei Shtylyov [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 --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +0200 [...] @@ -719,7 +719,7 @@ static long pdc_detect_pll_input_clock(s usec_elapsed = (end_time.tv_sec - start_time.tv_sec) * 100 + (end_time.tv_usec - start_time.tv_usec); - pll_clock = (start_count -
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 [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] 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.c2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +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) 0x; - bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; + 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) 0x; - bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; + 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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 [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 --- linux-2.6.23-rc3/drivers/ata/pata_pdc2027x.c 2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +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) 0x; - bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; + 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) 0x; - bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; + 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) * 100 + (end_time.tv_usec - start_time.tv_usec); - pll_clock = (start_count - end_count) / 100 * + pll_clock = ((start_count - end_count) 0x3fff) / 100 * (1 / 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 * (1 / 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 [EMAIL PROTECTED] -- albert - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 [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] 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.c2007-07-09 22:01:31.0 +0200 +++ linux-2.6.23-rc3.pata_pdc2027x-pll-detection-fixes/drivers/ata/pata_pdc2027x.c 2007-08-18 21:53:40.0 +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) 0x; -bccrh = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; +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) 0x; -bccrhv = readl(mmio_base + PDC_BYTE_COUNT + 0x100) 0x; +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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2.6.23-rc3] pata_pdc2027x: PLL detection fixes
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
