hw i2c was only implemented for r1xx-5xx asics; support for newer asics was never added.  The prescale value is not calculated reliably on certain pre-r5xx asics which lead to unreliable operation.

(In reply to comment #1)
> The prescale value is not calculated reliably on certain pre-r5xx
> asics which lead to unreliable operation.

I since tested the hw_i2c parameter with my rv280, and it fails here too. However, in this particular case I was able to "tweak" the algorithm so that it did work:

--- radeon_i2c.c.orig	2011-02-22 13:07:13.000000000 +0000
+++ radeon_i2c.c	2011-02-22 13:12:55.000000000 +0000
@@ -235,7 +235,7 @@
 		i2c_clock = 60;
 		nm = (sclk * 10) / (i2c_clock * 4);
 		for (loop = 1; loop < 255; loop++) {
-			if ((nm / loop) < loop)
+			if ((nm / loop) <= loop)
 				break;
 		}
 		n = loop - 1;

The working values were: sclk=25000, n=31, m=30. When I tried this exact same patch with my RV350, it did work *once*. However, I have not been able to repeat that success.

Can you point me to some context for this algorithm, please?

Created attachment 48662 [details]
dmesg output from RV350 on single transient success with hw_i2c=1

This dmesg output from my serial console shows that changing the "< loop" to "<= loop" did work on one occasion. However, it didn't last for long, and had started spitting out EDID errors again by the time I rebooted.

The hw i2c clock is derived from the engine clock on r1xx-r5xx asics.  The prescale scales it down from the engine clock to something useful for i2c.  I'm not sure exactly how the prescale is derived beyond the algorithms.

So the correct prescale value is really "whatever magic number happens to make it work"? Because the algorithm wouldn't need tweaking if it were truly definitive. Does it have a basis in any released documentation, and which asics is it currently known to work for, please?

HW i2c works on every card family I tested (rv100, rv280, rv380, rv410, r430, rv515, rv530) when I wrote the code.  The only chips that have reported problems are the oldest ones (the pre-rv380 chips; r100-rv350).  There isn't any documentation that I know of any more for how to calculate the prescale on these older asics; I dug up some old r100 sample code with the algo in it and it worked for me on the chips in question, but internally at AMD, no one has looked at this stuff in 8-10 years so info about it is pretty scarce.

You might try adjusting the i2c_clock value as well; it could be that some chips preferred slightly faster/slower hw i2c clocks.

(In reply to comment #6)
> The only chips that have reported problems are the oldest ones
> (the pre-rv380 chips; r100-rv350).

No coincidence that these are all the asics that calculate the prescale value using the for-loop algorithm, eh ;-)? (I have tested hw_i2c with my M66GL and it's fine, BTW.)

So it looks like "the trick" is to find the value of n such that ((n - 1) | (n << 8)) "works". It looks like n = 31 for my rv280, but not quite for my RV350. Both these cards have sclk = 25000, and so this algorithm would imply that there should exist a single value of n that works for both these cards too, correct?

(In reply to comment #7)
> 
> No coincidence that these are all the asics that calculate the prescale value
> using the for-loop algorithm, eh ;-)? (I have tested hw_i2c with my M66GL and
> it's fine, BTW.)
> 
> So it looks like "the trick" is to find the value of n such that ((n - 1) |
> (n
> << 8)) "works". It looks like n = 31 for my rv280, but not quite for my
> RV350.
> Both these cards have sclk = 25000, and so this algorithm would imply that
> there should exist a single value of n that works for both these cards too,
> correct?

The algo I found was for r100 IIRC, but it happened to work for me on r1xx, r2xx, and r3xx, so I went with it.  I wasn't able to find any info specially about r2xx or r3xx.  It's possible that different asics need slightly different algos.

If you look at the rv380-rv410 and r5xx algos, they are actually pretty similar to the loop one, only they use a a fixed value of 128 for m and 100 for the i2c_clock. e.g.,

diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index ded2a45..734b1ef 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -248,7 +248,10 @@ static u32 radeon_get_i2c_prescale(struct radeon_device *rdev)
        case CHIP_R420:
        case CHIP_R423:
        case CHIP_RV410:
-               prescale = (((sclk * 10)/(4 * 128 * 100) + 1) << 8) + 128;
+               i2c_clock = 100;
+               m = 128;
+               n = ((sclk * 10) / (4 * 128 * i2c_clock)) + 1;
+               prescale = m | (n << 8);
                break;
        case CHIP_RS600:
        case CHIP_RS690:
@@ -261,11 +264,17 @@ static u32 radeon_get_i2c_prescale(struct radeon_device *rdev)
        case CHIP_RV560:
        case CHIP_RV570:
        case CHIP_R580:
-               i2c_clock = 50;
-               if (rdev->family == CHIP_R520)
-                       prescale = (127 << 8) + ((sclk * 10) / (4 * 127 * i2c_clock));
-               else
-                       prescale = (((sclk * 10)/(4 * 128 * 100) + 1) << 8) + 128;
+               if (rdev->family == CHIP_R520) {
+                       i2c_clock = 50;
+                       m = (sclk * 10) / (4 * 127 * i2c_clock);
+                       n = 127;
+                       prescale = m | (n << 8);
+               } else {
+                       i2c_clock = 100;
+                       m = 128;
+                       n = ((sclk * 10) / (4 * 128 * i2c_clock)) + 1;
+                       prescale = m | (n << 8);
+               }
                break;
        case CHIP_R600:
        case CHIP_RV610:

I've just tested my RV350 and discovered that hw_i2c *seems* to work for 11 <= n <= 30. IOW, I get a picture on my DVI monitor without any scary errors in the dmesg log.

For my RV280, hw_i2c works for 12 <= n <= 31. I still have my rv100 to test as well.

My rv100 has a sclk value of 13300, and I have (so far) been unable to find a value of n that will enable HW I2C.

Created attachment 49322 [details]
dmesg output showing I2C errors with rv100 and DVI monitor

Actually, it looks like 2.6.37.2 has deeper issues with rv100 and I2C, beyond merely hw_i2c. Device i2c-0/name is:

Radeon i2c bit bus DVI_DDC

Is this still a problem ?

(In reply to comment #13)
> Is this still a problem ?

Yes, but I have been patching my kernel locally so that it works for my RV280 and RV350 cards. (I never did manage to fix it for my rv100, though).

Without the patch, enabling hw_i2c results in a lot of this:

[   44.954542] Raw EDID:
[   44.956826]      00 ff ff ff ff ff ff 00 ff ff ff ff ff ff ff ff
[   44.962576]      ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   44.968328]      ff ff ff ff 00 ff ff 1e ff ff 6d 6d ff ff 3e 3e
[   44.974080]      ff ff ff 4e 4e ff ff ff 9a 9a ff ff ff f7 f7 ff
[   44.979830]      ff ff 02 02 ff ff 00 00 ff ff ff ff ff ff ff 08
[   44.985580]      08 ff ff 10 10 ff ff ff ff 01 01 ff ff ff 03 03
[   44.991330]      ff ff ff ee ee ff ff ff 2b 2b ff ff ff ff 1b 1b
[   44.997080]      ff ff ff ff ff ff ff 78 78 ff ff ea ff ff c6 ff
[   45.002836] radeon 0000:01:00.0: DVI-I-1: EDID block 0 invalid.
[   45.008765] [drm:radeon_dvi_detect] *ERROR* DVI-I-1: probed a monitor but no|invalid EDID

And no monitor output, of course.

Created attachment 77871 [details]
Fixes hw_i2c for RV350 and RV280 cards.

Testing whether this fixes any other cases would be good, although I can confirm that it *doesn't* fix the rv100 case.

Signed-off-by: Chris Rankin <rankincj@yahoo.com>