radeonfb: Let hwmon driver probe the "monid" I2C bus

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

Reported-by: Alexander Goomenyuk <redacted>
Signed-off-by: Jean Delvare <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Ben, please let me know if you have any objection, and if you don't,
please let me know if you want to pick this patch or if you prefer that
I push it through my hwmon tree.

 drivers/video/aty/radeon_i2c.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.38-rc0.orig/drivers/video/aty/radeon_i2c.c	2011-01-05 21:24:24.000000000 +0100
+++ linux-2.6.38-rc0/drivers/video/aty/radeon_i2c.c	2011-01-11 13:51:34.000000000 +0100

@@ -100,6 +100,7 @@ void radeon_create_i2c_busses(struct rad
 {
 	rinfo->i2c[0].rinfo	= rinfo;
 	rinfo->i2c[0].ddc_reg	= GPIO_MONID;
+	rinfo->i2c[0].adapter.class = I2C_CLASS_HWMON;
 	radeon_setup_i2c_bus(&rinfo->i2c[0], "monid");
 
 	rinfo->i2c[1].rinfo	= rinfo;

-- 
Jean Delvare

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

Reported-by: Alexander Goomenyuk <redacted>
Signed-off-by: Jean Delvare <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Ben, please let me know if you have any objection, and if you don't,
please let me know if you want to pick this patch or if you prefer that
I push it through my hwmon tree.

I can take this through the fbdev tree if Ben's happy with it.

On Thu, Jan 13, 2011 at 2:11 PM, Paul Mundt [off-list ref] wrote:

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

quoted

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

The radeon BIOS actually contains a table to tell us what chip is on the bus,
in the KMS driver we read this table and load the appropriate hwmon device,

There could possibly be other things on this bus than hw monitor devices.

Though radeonfb should be considered mostly dead.

Dave.

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

The radeon BIOS actually contains a table to tell us what chip is on
the bus,
in the KMS driver we read this table and load the appropriate hwmon
device,

There could possibly be other things on this bus than hw monitor
devices.

Though radeonfb should be considered mostly dead. 

I want to test that on a few powermacs first. "probing" i2c can be
destructive and f* knows what Apple put on those busses on some
machines.

Haven't had a chance to look much yet.

Cheers,
Ben.

On Thu, 13 Jan 2011 14:44:53 +1000, Dave Airlie wrote:

On Thu, Jan 13, 2011 at 2:11 PM, Paul Mundt [off-list ref] wrote:

quoted

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

quoted

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

The radeon BIOS actually contains a table to tell us what chip is on the bus,
in the KMS driver we read this table and load the appropriate hwmon device,

There could possibly be other things on this bus than hw monitor devices.

I know that, but...

Though radeonfb should be considered mostly dead.

... I doubt anybody is interested in investing that much worktime in
radeonfb any longer.

-- 
Jean Delvare

Hi Ben,

On Thu, 13 Jan 2011 15:50:10 +1100, Benjamin Herrenschmidt wrote:

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

quoted

The radeon BIOS actually contains a table to tell us what chip is on
the bus,
in the KMS driver we read this table and load the appropriate hwmon
device,

There could possibly be other things on this bus than hw monitor
devices.

Though radeonfb should be considered mostly dead. 

I want to test that on a few powermacs first. "probing" i2c can be
destructive and f* knows what Apple put on those busses on some
machines.

If this is a concern to you, I'd be fine excluding CONFIG_PPC from the
change. Or are you worried about Intel Mac machines too? If there's an
easy way to spot all powermac machines, we can use that too, just let
me know how.

Haven't had a chance to look much yet.

Thanks,
-- 
Jean Delvare

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

On Thu, Jan 13, 2011 at 2:11 PM, Paul Mundt [off-list ref] wrote:

quoted

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

quoted

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

The radeon BIOS actually contains a table to tell us what chip is on the bus,
in the KMS driver we read this table and load the appropriate hwmon device,

There could possibly be other things on this bus than hw monitor devices.

Though radeonfb should be considered mostly dead.

Dave.

I have tried radeon KMS driver.
        modinfo output:
        filename:       /lib/modules/2.6.36-tuxonice-r3/kernel/drivers/gpu/drm/radeon/radeon.ko
        depends:
        drm,drm_kms_helper,ttm,fb,i2c-core,cfbfillrect,cfbimgblt,cfbcopyarea,i2c-algo-bit
        vermagic:       2.6.36-tuxonice-r3 SMP mod_unload 686 
        parm:           no_wb:Disable AGP writeback for scratch
        registers (int)
        parm:           modeset:Disable/Enable modesetting (int)
        parm:           dynclks:Disable/Enable dynamic clocks (int)
        parm:           r4xx_atom:Enable ATOMBIOS modesetting for R4xx
        (int)
        parm:           vramlimit:Restrict VRAM for testing (int)
        parm:           agpmode:AGP Mode (-1 = PCI) (int)
        parm:           gartsize:Size of PCIE/IGP gart to setup in
        megabytes (32,64, etc)
         (int)
        parm:           benchmark:Run benchmark (int)
        parm:           test:Run tests (int)
        parm:           connector_table:Force connector table (int)
        parm:           tv:TV enable (0 = disable) (int)
        parm:           new_pll:Select new PLL code (int)
        parm:           audio:Audio enable (0 = disable) (int)
        parm:           disp_priority:Display Priority (0 = auto, 1         normal, 2 = high) (int)
        parm:           hw_i2c:hw i2c engine enable (0 = disable) (int)
        
But it also need modifications in order to be used by lm63 hwmon driver.
1. MONID line is disable by combios_setup_i2c_bus() for RV380 because
of:
        case CHIP_RV350:
        case CHIP_RV380:
        case CHIP_RS400:
        case CHIP_RS480:
                switch (ddc_line) {
                case RADEON_GPIO_VGA_DDC:
                case RADEON_GPIO_DVI_DDC:
+                       i2c.hw_capable = true;
-                       i2c.hw_capable = false;
                        break;

25 more lines

Alexander,

On Wed, 02 Feb 2011 07:50:16 +0200, Alexander Goomenyuk wrote:

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

quoted

On Thu, Jan 13, 2011 at 2:11 PM, Paul Mundt [off-list ref] wrote:

quoted

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

quoted

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

The radeon BIOS actually contains a table to tell us what chip is on the bus,
in the KMS driver we read this table and load the appropriate hwmon device,

There could possibly be other things on this bus than hw monitor devices.

Though radeonfb should be considered mostly dead.

Dave.

I have tried radeon KMS driver.
        modinfo output:
        filename:       /lib/modules/2.6.36-tuxonice-r3/kernel/drivers/gpu/drm/radeon/radeon.ko
        depends:
        drm,drm_kms_helper,ttm,fb,i2c-core,cfbfillrect,cfbimgblt,cfbcopyarea,i2c-algo-bit
        vermagic:       2.6.36-tuxonice-r3 SMP mod_unload 686 
        parm:           no_wb:Disable AGP writeback for scratch
        registers (int)
        parm:           modeset:Disable/Enable modesetting (int)
        parm:           dynclks:Disable/Enable dynamic clocks (int)
        parm:           r4xx_atom:Enable ATOMBIOS modesetting for R4xx
        (int)
        parm:           vramlimit:Restrict VRAM for testing (int)
        parm:           agpmode:AGP Mode (-1 = PCI) (int)
        parm:           gartsize:Size of PCIE/IGP gart to setup in
        megabytes (32,64, etc)
         (int)
        parm:           benchmark:Run benchmark (int)
        parm:           test:Run tests (int)
        parm:           connector_table:Force connector table (int)
        parm:           tv:TV enable (0 = disable) (int)

28 more quoted lines

I'm sorry but I don't understand what you mean. For one thing, I can't
see how the above change (?) is related to your problem, as it only
affects the DDC channels and not the MONID channel. For another, I
can't see how the bus being software or hardware driven would have any
effect.

2. MONID adapter is not registered as HWMON capable.
So I have added the following line into radeon_combios_i2c_init():

        } else if (rdev->family >= CHIP_R300) {
               i2c = combios_setup_i2c_bus(rdev, DDC_MONID, 0, 0);
               rdev->i2c_bus[3] = radeon_i2c_create(dev, &i2c, "MONID");
+              rdev->i2c_bus[3]->adapter.class = I2C_CLASS_HWMON;

Now I see GPU temperature using lm_sensors.

But this isn't the way the drm radeon driver is designed. It reads from
the BIOS whether there are sensors available, what is their type and
their address, etc. It does not rely on physical probing as the
radeonfb driver does. So the above is NOT correct.

I can imagine that sensor support has only been implemented for
atom-BIOS based boards and not older com-BIOS based ones. At least
searching for "lm63" in the source code has matches in only
radeon_atombios.c and not radeon_combios.c. I have no idea if it would
be possible to extend this mechanism to older boards or not - I don't
know if the com-BIOS included the information already.

-- 
Jean Delvare

On Thu, 13 Jan 2011 09:56:57 +0100, Jean Delvare wrote:

Hi Ben,

On Thu, 13 Jan 2011 15:50:10 +1100, Benjamin Herrenschmidt wrote:

quoted

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

quoted

The radeon BIOS actually contains a table to tell us what chip is on
the bus,
in the KMS driver we read this table and load the appropriate hwmon
device,

There could possibly be other things on this bus than hw monitor
devices.

Though radeonfb should be considered mostly dead. 

I want to test that on a few powermacs first. "probing" i2c can be
destructive and f* knows what Apple put on those busses on some
machines.

If this is a concern to you, I'd be fine excluding CONFIG_PPC from the
change. Or are you worried about Intel Mac machines too? If there's an
easy way to spot all powermac machines, we can use that too, just let
me know how.

quoted

Haven't had a chance to look much yet.

Ben, any progress on this?

-- 
Jean Delvare

On Wed, 2011-02-02 at 09:12 +0100, Jean Delvare wrote:

quoted

If this is a concern to you, I'd be fine excluding CONFIG_PPC from

the

quoted

change. Or are you worried about Intel Mac machines too? If there's

an

quoted

easy way to spot all powermac machines, we can use that too, just

let

quoted

me know how.

quoted

Haven't had a chance to look much yet.

Ben, any progress on this?

Sorry, too swamped. Go on with CONFIG_PPC excluded for now.

Cheers,
Ben.

On Mit, 2011-02-02 at 07:50 +0200, Alexander Goomenyuk wrote: 

P.S. There is also strange side effect after reloading radeon driver
from X console: the screen switch off. So I have to switch between
virtual console and X. Is it an bug?

I think there's a bug where unloading the radeon kernel module without
KMS will interfere with the hardware state for the X driver. However,
you can only unload the radeon kernel module if the X driver uses
neither KMS nor DRI in general, which is undesirable in the first place.
So fixing this is low priority even as far as UMS bugs are concerned.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer

Thanks for fast reply.

I'm sorry but I don't understand what you mean. For one thing, I can't
see how the above change (?) is related to your problem, as it only
affects the DDC channels and not the MONID channel. For another, I
can't see how the bus being software or hardware driven would have any
effect.

Sorry, that was my mistake (artifact left after experiments).

But this isn't the way the drm radeon driver is designed. It reads

from

the BIOS whether there are sensors available, what is their type and
their address, etc. It does not rely on physical probing as the
radeonfb driver does. So the above is NOT correct.

I can imagine that sensor support has only been implemented for
atom-BIOS based boards and not older com-BIOS based ones. At least
searching for "lm63" in the source code has matches in only
radeon_atombios.c and not radeon_combios.c. I have no idea if it would
be possible to extend this mechanism to older boards or not - I don't
know if the com-BIOS included the information already.

I would like to make some clarifications:
The intention was to check how new radeon is working, or not working :),
with thermal sensors. I am not competent enough to introduce any fixes
to radeon driver.

The question is: Should I file the bug for radeon driver to implement
thermal sensor support for com-BIOS based cards or not?

On Wed, 2011-02-02 at 09:11 +0100, Jean Delvare wrote:

Alexander,

On Wed, 02 Feb 2011 07:50:16 +0200, Alexander Goomenyuk wrote:

quoted

On Thu, 2011-01-13 at 14:44 +1000, Dave Airlie wrote:

quoted

On Thu, Jan 13, 2011 at 2:11 PM, Paul Mundt [off-list ref] wrote:

quoted

On Tue, Jan 11, 2011 at 04:23:52PM +0100, Jean Delvare wrote:

quoted

Some Radeon cards have an I2C-based thermal sensor chip connected to
the "monid" I2C bus. Set the I2C probing class of this bus properly so
that hwmon drivers can detect devices on it and bind to them.

This closes kernel.org bug #26172.

The radeon BIOS actually contains a table to tell us what chip is on the bus,
in the KMS driver we read this table and load the appropriate hwmon device,

There could possibly be other things on this bus than hw monitor devices.

Though radeonfb should be considered mostly dead.

Dave.

I have tried radeon KMS driver.
        modinfo output:
        filename:       /lib/modules/2.6.36-tuxonice-r3/kernel/drivers/gpu/drm/radeon/radeon.ko
        depends:
        drm,drm_kms_helper,ttm,fb,i2c-core,cfbfillrect,cfbimgblt,cfbcopyarea,i2c-algo-bit
        vermagic:       2.6.36-tuxonice-r3 SMP mod_unload 686 
        parm:           no_wb:Disable AGP writeback for scratch
        registers (int)
        parm:           modeset:Disable/Enable modesetting (int)
        parm:           dynclks:Disable/Enable dynamic clocks (int)
        parm:           r4xx_atom:Enable ATOMBIOS modesetting for R4xx
        (int)
        parm:           vramlimit:Restrict VRAM for testing (int)
        parm:           agpmode:AGP Mode (-1 = PCI) (int)
        parm:           gartsize:Size of PCIE/IGP gart to setup in
        megabytes (32,64, etc)
         (int)
        parm:           benchmark:Run benchmark (int)

61 more quoted lines

On Thu, 03 Feb 2011 07:41:01 +0200, Alexander Goomenyuk wrote:

quoted

I can imagine that sensor support has only been implemented for
atom-BIOS based boards and not older com-BIOS based ones. At least
searching for "lm63" in the source code has matches in only
radeon_atombios.c and not radeon_combios.c. I have no idea if it would
be possible to extend this mechanism to older boards or not - I don't
know if the com-BIOS included the information already.

I would like to make some clarifications:
The intention was to check how new radeon is working, or not working :),
with thermal sensors. I am not competent enough to introduce any fixes
to radeon driver.

And neither am I.

The question is: Should I file the bug for radeon driver to implement
thermal sensor support for com-BIOS based cards or not?

This would obviously be a feature request rather than a bug report.

I don't know if the com-BIOS based cards have the thermal sensor
information in the first place, so I don't know if the feature can be
implemented. If they don't, then auto-detection is the way to go, and
we can do it ourselves. If they do, the radeon driver maintainers would
have to work on it, if they are able and willing to do so.

So the next step is the radeon driver maintainers telling us whether
com-BIOS based cards have thermal sensor information as atom-BIOS based
ones do. You should ask this question to the right people and list.

-- 
Jean Delvare