I2C: fsl-i2c: make device probing configurable via FDT

RFC In review Wolfgang Grandegger 7 participants 14 messages Jul 16, 2008 – Jul 17, 2008

Subsystems (3)

PowerPC I2C subsystem host drivers Freescale mpc I2C driver

lore

Share & tools

lore

Wolfgang Grandegger

author

2008-07-16 10:47 UTC #0 lore raw

RFC 1/1 I2C: fsl-i2c: make device probing configurable via FDT

Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:

      i2c@3000 {
           ...
           compatible = "fsl-i2c";
           probe;
	    ...
       };

Assuming that systems using fsl-i2c should have proper platform data, probing is disabled by default. Unfortunately, that's not the case and various DTS files would require to be updated.

There have already been some discussion on this subject with Jean under the following thread:

http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html

Please comment.

Thanks,

Wolfgang.

Signed-off-by Wolfgang Grandegger <[email protected]>

---

arch/powerpc/sysdev/fsl_soc.c |    4 ++++
drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
include/linux/fsl_devices.h   |    1 +
3 files changed, 19 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-mpc.c +0 -0

Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
===================================================================
@@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al

.functionality = mpc_functionality,
};

-static struct i2c_adapter mpc_ops = {
+static struct i2c_adapter mpc_probe_ops = {
.owner = THIS_MODULE,
.name = "MPC adapter",
.id = I2C_HW_MPC107,
.algo = &mpc_algo,
.class = I2C_CLASS_HWMON,
- .timeout = 1,
+ .timeout = 1,
+};
+
+static struct i2c_adapter mpc_ops = {
+ .owner = THIS_MODULE,
+ .name = "MPC adapter",
+ .id = I2C_HW_MPC107,
+ .algo = &mpc_algo,
+ .timeout = 1,
};

static int fsl_i2c_probe(struct platform_device *pdev) @@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform

	mpc_i2c_setclock(i2c);
	platform_set_drvdata(pdev, i2c);

- i2c->adap = mpc_ops;
+ (i2c->flags & FSL_I2C_DEV_PROBE)
+ i2c->adap = mpc_probe_ops;
+ else

+		i2c->adap = mpc_ops;
	i2c->adap.nr = pdev->id;
	i2c_set_adapdata(&i2c->adap, i2c);
	i2c->adap.dev.parent = &pdev->dev;

arch/powerpc/sysdev/fsl_soc.c +0 -0

Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
===================================================================
@@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)

		if (flags)
			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;

+ flags = of_get_property(np, "probe", NULL);
+ if (flags)
+ i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
+
ret =
platform_device_add_data(i2c_dev, &i2c_data,
sizeof(struct

include/linux/fsl_devices.h +0 -0

Index: linux-2.6-denx/include/linux/fsl_devices.h
===================================================================
@@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {

/* Flags related to I2C device features */
#define FSL_I2C_DEV_SEPARATE_DFSRR 0x00000001
#define FSL_I2C_DEV_CLOCK_5200 0x00000002
+#define FSL_I2C_DEV_PROBE 0x00000004

enum fsl_usb2_operating_modes {
	FSL_USB2_MPH_HOST,

Jean Delvare

2008-07-16 12:05 UTC #1 lore raw

Hi Wolfgang,

Quoted 40 lines · Wolfgang Grandegger

Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
       i2c@3000 {
            ...
            compatible = "fsl-i2c";
            probe;
	    ...
        };
Assuming that systems using fsl-i2c should have proper platform data, probing is disabled by default. Unfortunately, that's not the case and various DTS files would require to be updated.
There have already been some discussion on this subject with Jean under the following thread:
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html
Please comment.
Thanks,
Wolfgang.
Signed-off-by Wolfgang Grandegger <[email protected]>
---
 arch/powerpc/sysdev/fsl_soc.c |    4 ++++
 drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
 include/linux/fsl_devices.h   |    1 +
 3 files changed, 19 insertions(+), 3 deletions(-)
Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
===================================================================
drivers/i2c/busses/i2c-mpc.c +0 -0
@@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al
 	.functionality = mpc_functionality,
 };

drivers/i2c/busses/i2c-mpc.c static const struct i2c_algorithm mpc_al · line 315

Quoted 10 lines

drivers/i2c/busses/i2c-mpc.c +2 -2

 
-static struct i2c_adapter mpc_ops = {
+static struct i2c_adapter mpc_probe_ops = {
 	.owner = THIS_MODULE,
 	.name = "MPC adapter",
 	.id = I2C_HW_MPC107,
 	.algo = &mpc_algo,
 	.class = I2C_CLASS_HWMON,
-	.timeout = 1,
+ 	.timeout = 1,

Your patch is adding undesirable leading white space.

drivers/i2c/busses/i2c-mpc.c static const struct i2c_algorithm mpc_al · line 324

Quoted 9 lines

drivers/i2c/busses/i2c-mpc.c +8 -0

+};
+
+static struct i2c_adapter mpc_ops = {
+	.owner = THIS_MODULE,
+	.name = "MPC adapter",
+	.id = I2C_HW_MPC107,
+	.algo = &mpc_algo,
+ 	.timeout = 1,
 };

I don't think you need 2 separate structures. Just set the class field at runtime (see below.)

drivers/i2c/busses/i2c-mpc.c static int fsl_i2c_probe(struct platform · line 365

Quoted 8 lines

drivers/i2c/busses/i2c-mpc.c +1 -1

 static int fsl_i2c_probe(struct platform_device *pdev)

@@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform
 	mpc_i2c_setclock(i2c);
 	platform_set_drvdata(pdev, i2c);
 
-	i2c->adap = mpc_ops;
+	(i2c->flags & FSL_I2C_DEV_PROBE)

Missing if? :)

drivers/i2c/busses/i2c-mpc.c static int fsl_i2c_probe(struct platform · line 368

Quoted 3 lines

drivers/i2c/busses/i2c-mpc.c +3 -0
+		i2c->adap = mpc_probe_ops;
+	else
+		i2c->adap = mpc_ops;

Or just:

	i2c->adap = mpc_ops;
	if (i2c->flags & FSL_I2C_DEV_PROBE)
		i2c->adap.flags = I2C_CLASS_HWMON;

Quoted 23 lines

drivers/i2c/busses/i2c-mpc.c +0 -0
 	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;
Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
===================================================================
arch/powerpc/sysdev/fsl_soc.c +4 -0
@@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)
 		if (flags)
 			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
 
+		flags = of_get_property(np, "probe", NULL);
+		if (flags)
+			i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
+
 		ret =
 		    platform_device_add_data(i2c_dev, &i2c_data,
 					     sizeof(struct
Index: linux-2.6-denx/include/linux/fsl_devices.h
===================================================================

include/linux/fsl_devices.h struct fsl_i2c_platform_data { · line 88

Quoted 7 lines

include/linux/fsl_devices.h +1 -0

@@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {
 /* Flags related to I2C device features */
 #define FSL_I2C_DEV_SEPARATE_DFSRR	0x00000001
 #define FSL_I2C_DEV_CLOCK_5200		0x00000002
+#define FSL_I2C_DEV_PROBE		0x00000004
 
 enum fsl_usb2_operating_modes {
 	FSL_USB2_MPH_HOST,

As a side note: in general it is not a good idea to use a template for struct i2c_adapter. This structure is relatively large compared to the few fields you need to set. If you insist on having a template, it should at least be marked either const or __initdata.

Signature · 1 line

Jean Delvare

Wolfgang Grandegger

author

2008-07-16 13:03 UTC #2 lore raw

Quoted 75 lines · Jean Delvare

Hi Wolfgang,
Quoted 40 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
       i2c@3000 {
            ...
            compatible = "fsl-i2c";
            probe;
	    ...
        };
Assuming that systems using fsl-i2c should have proper platform data, probing is disabled by default. Unfortunately, that's not the case and various DTS files would require to be updated.
There have already been some discussion on this subject with Jean under the following thread:
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060008.html
Please comment.
Thanks,
Wolfgang.
Signed-off-by Wolfgang Grandegger <[email protected]>
---
 arch/powerpc/sysdev/fsl_soc.c |    4 ++++
 drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
 include/linux/fsl_devices.h   |    1 +
 3 files changed, 19 insertions(+), 3 deletions(-)
Index: linux-2.6-denx/drivers/i2c/busses/i2c-mpc.c
===================================================================
drivers/i2c/busses/i2c-mpc.c +0 -0
@@ -306,13 +306,21 @@ static const struct i2c_algorithm mpc_al
 	.functionality = mpc_functionality,
 };
drivers/i2c/busses/i2c-mpc.c static const struct i2c_algorithm mpc_al · line 315
Quoted 10 lines
drivers/i2c/busses/i2c-mpc.c +2 -2
 
-static struct i2c_adapter mpc_ops = {
+static struct i2c_adapter mpc_probe_ops = {
 	.owner = THIS_MODULE,
 	.name = "MPC adapter",
 	.id = I2C_HW_MPC107,
 	.algo = &mpc_algo,
 	.class = I2C_CLASS_HWMON,
-	.timeout = 1,
+ 	.timeout = 1,
Your patch is adding undesirable leading white space.
drivers/i2c/busses/i2c-mpc.c static const struct i2c_algorithm mpc_al · line 324
Quoted 9 lines
drivers/i2c/busses/i2c-mpc.c +8 -0
+};
+
+static struct i2c_adapter mpc_ops = {
+	.owner = THIS_MODULE,
+	.name = "MPC adapter",
+	.id = I2C_HW_MPC107,
+	.algo = &mpc_algo,
+ 	.timeout = 1,
 };
I don't think you need 2 separate structures. Just set the class field at runtime (see below.)
Quoted 6 lines
drivers/i2c/busses/i2c-mpc.c +0 -0
 static int fsl_i2c_probe(struct platform_device *pdev)
@@ -354,7 +362,10 @@ static int fsl_i2c_probe(struct platform
 	mpc_i2c_setclock(i2c);
 	platform_set_drvdata(pdev, i2c);
 

Quoted 4 lines

drivers/i2c/busses/i2c-mpc.c static int fsl_i2c_probe(struct platform · line 365
Quoted 2 lines
drivers/i2c/busses/i2c-mpc.c +1 -1
-	i2c->adap = mpc_ops;
+	(i2c->flags & FSL_I2C_DEV_PROBE)
Missing if? :)

Oops, I obviously forgot the quilt refresh before sending.

Quoted 10 lines

drivers/i2c/busses/i2c-mpc.c static int fsl_i2c_probe(struct platform · line 368
Quoted 4 lines
drivers/i2c/busses/i2c-mpc.c +3 -0
+		i2c->adap = mpc_probe_ops;
+	else
+		i2c->adap = mpc_ops;
Or just:
	i2c->adap = mpc_ops;
	if (i2c->flags & FSL_I2C_DEV_PROBE)
		i2c->adap.flags = I2C_CLASS_HWMON;

Right, mpc_ops is not a pointer, as I realize now.

Quoted 28 lines

drivers/i2c/busses/i2c-mpc.c +0 -0

 	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;

Index: linux-2.6-denx/arch/powerpc/sysdev/fsl_soc.c
===================================================================

arch/powerpc/sysdev/fsl_soc.c +4 -0

@@ -518,6 +518,10 @@ static int __init fsl_i2c_of_init(void)
 		if (flags)
 			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
 
+		flags = of_get_property(np, "probe", NULL);
+		if (flags)
+			i2c_data.device_flags |= FSL_I2C_DEV_PROBE;
+
 		ret =
 		    platform_device_add_data(i2c_dev, &i2c_data,
 					     sizeof(struct

Index: linux-2.6-denx/include/linux/fsl_devices.h
===================================================================

include/linux/fsl_devices.h +1 -0

@@ -82,6 +82,7 @@ struct fsl_i2c_platform_data {
 /* Flags related to I2C device features */
 #define FSL_I2C_DEV_SEPARATE_DFSRR	0x00000001
 #define FSL_I2C_DEV_CLOCK_5200		0x00000002
+#define FSL_I2C_DEV_PROBE		0x00000004

Quoted 7 lines

include/linux/fsl_devices.h struct fsl_i2c_platform_data { · line 88
Quoted 2 lines
include/linux/fsl_devices.h +0 -0
 enum fsl_usb2_operating_modes {
 	FSL_USB2_MPH_HOST,
As a side note: in general it is not a good idea to use a template for struct i2c_adapter. This structure is relatively large compared to the few fields you need to set. If you insist on having a template, it should at least be marked either const or __initdata.

Wolfgang.

Jon Smirl

2008-07-16 12:47 UTC #3 lore raw

In reply to Wolfgang Grandegger

Quoted 3 lines · Wolfgang Grandegger

Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:

All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.

Why do you need to do this? The OF conversion patch that is working its way through the system lets you put the address of the device into the device tree node. Probing shouldn't be necessary at all.

Signature · 2 lines

Jon Smirl
[email protected]

Wolfgang Grandegger

author

2008-07-16 13:09 UTC #4 lore raw

Quoted 7 lines · Jon Smirl

Quoted 3 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.

Yep.

Quoted 3 lines

Why do you need to do this? The OF conversion patch that is working its way through the system lets you put the address of the device into the device tree node. Probing shouldn't be necessary at all.

Currently I2C_CLASS_HWMON is used for all I2C adapters and therefore probing is done even with a proper I2C node defined in the DTS file. That's what the patch is fixing leaving the possibility to (re-)enable legacy probing.

For some more details you may want to have a look to Jean's mail on that subject: http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060012.html.

Wolfgang.

Jon Smirl

2008-07-16 14:11 UTC #5 lore raw

Quoted 21 lines · Wolfgang Grandegger

Quoted 10 lines · Jon Smirl
Quoted 4 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.
Yep.
Quoted 4 lines
Why do you need to do this? The OF conversion patch that is working its way through the system lets you put the address of the device into the device tree node. Probing shouldn't be necessary at all.

Quoted 4 lines

Currently I2C_CLASS_HWMON is used for all I2C adapters and therefore
probing is done even with a proper I2C node defined in the DTS file. That's what the patch is fixing leaving the possibility to (re-)enable legacy probing.

Shouldn't we just remove I2C_CLASS_HWMON since everything can now be explicitly described in the device tree? What is an example of a case where we still need to probe?

Quoted 9 lines

For some more details you may want to have a look to Jean's mail on that
subject:
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060012.html.
Wolfgang.

Signature · 2 lines

Jon Smirl
[email protected]

Jon Smirl

2008-07-16 14:24 UTC #6 lore raw

In reply to Wolfgang Grandegger

Quoted 11 lines · Wolfgang Grandegger

Quoted 9 lines · Jon Smirl
Quoted 4 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.

If this is implemented it shouldn't be a device tree option, it should be a compile time option in the Kconfig system. We don't want to pollute a platform independent device tree with Linux clutter.

Signature · 2 lines

Jon Smirl
[email protected]

Grant Likely

2008-07-16 14:30 UTC #7 lore raw

Quoted 10 lines · Jon Smirl

Quoted 9 lines · Wolfgang Grandegger
Quoted 7 lines · Jon Smirl
Quoted 4 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.
If this is implemented it shouldn't be a device tree option, it should be a compile time option in the Kconfig system. We don't want to pollute a platform independent device tree with Linux clutter.

I'm not sure about this. It is somewhat describing the hardware. It sounds like it is saying "hey, I've got an i2c bus here, but I don't know what is on it; so instead of providing a set of child nodes, you should probe this bus to find out what it has".

I don't see any problem specifying it in the device tree.

Making it a Kconfig option forces *all* i2c busses on supported platforms into either probe or no-probe mode. Not a multiplatform friendly solution. Heck, we've got multiple i2c busses on some chips and it is conceivable that only one would need to be probed.

Cheers,
g.

Jon Smirl

2008-07-16 14:42 UTC #8 lore raw

Quoted 19 lines · Grant Likely

Quoted 10 lines · Jon Smirl
Quoted 9 lines · Wolfgang Grandegger
Quoted 7 lines · Jon Smirl
Quoted 4 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might
not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
All this patch seems to be doing is removing class I2C_CLASS_HWMON via a device tree flag, PROBE.
If this is implemented it shouldn't be a device tree option, it should be a compile time option in the Kconfig system. We don't want to pollute a platform independent device tree with Linux clutter.

Quoted 4 lines

I'm not sure about this. It is somewhat describing the hardware. It
sounds like it is saying "hey, I've got an i2c bus here, but I don't know what is on it; so instead of providing a set of child nodes, you should probe this bus to find out what it has".

Probing an i2c bus does not necessarily come to a good conclusion. The probes for some chips can alter the states in others. People have accidentally changed voltage settings and fried CPU chips. The process is not well defined.

I'm not saying remove i2c probing altogether, some things require probing like EDID data from a monitor. Since these are embedded system I just think it is pretty unlikely that stuff like that is going to get hooked to an i2c-mpc bus. That was behind the idea to push it back to a compile time option.

Quoted 12 lines

I don't see any problem specifying it in the device tree.
Making it a Kconfig option forces *all* i2c busses on supported platforms into either probe or no-probe mode. Not a multiplatform friendly solution. Heck, we've got multiple i2c busses on some chips and it is conceivable that only one would need to be probed.
Cheers,
g.

Signature · 2 lines

Jon Smirl
[email protected]

Timur Tabi

2008-07-16 15:01 UTC #9 lore raw

Quoted 4 lines · Jon Smirl

Probing an i2c bus does not necessarily come to a good conclusion. The probes for some chips can alter the states in others. People have accidentally changed voltage settings and fried CPU chips. The process is not well defined.

I agree. We should not be implementing any feature that encourages I2C bus probing.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Grant Likely

2008-07-16 14:24 UTC #10 lore raw

In reply to Wolfgang Grandegger

Quoted 10 lines · Wolfgang Grandegger

Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
      i2c@3000 {
           ...
           compatible = "fsl-i2c";
           probe;
	    ...
       };

You need to add documentation to booting-without-of.txt about what the 'probe' property means. Also, 'probe' is a pretty generic term being used in an i2c specific context. Can you prefix it with i2c or something to protect the namespace? 'i2c-probe' perhaps?

Quoted 3 lines

Assuming that systems using fsl-i2c should have proper platform data, probing is disabled by default. Unfortunately, that's not the case and various DTS files would require to be updated.

This comment doesn't seem to be correct.

Quoted 4 lines

---

arch/powerpc/sysdev/fsl_soc.c |    4 ++++
drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
include/linux/fsl_devices.h   |    1 +

This will need to be reworked for current top of tree. i2c-mpc dependence on fsl_soc.c has been removed.

Thanks,
g.

Jochen Friedrich

2008-07-16 14:48 UTC #11 lore raw

Hi Grant,

Quoted 12 lines · Wolfgang Grandegger

Quoted 10 lines
Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
      i2c@3000 {
           ...
           compatible = "fsl-i2c";
           probe;
	    ...
       };

Quoted 4 lines

You need to add documentation to booting-without-of.txt about what the 'probe' property means. Also, 'probe' is a pretty generic term being used in an i2c specific context. Can you prefix it with i2c or something to protect the namespace? 'i2c-probe' perhaps?

In i2c-cpm, I currently use the property "linux,i2c-class" to set the i2c class directly as documented in dts-bindings/fsl/cpm_qe/cpm/i2c.txt.

We should agree on *one* method to set the i2c class.

Thanks,
Jochen

Wolfgang Grandegger

author

2008-07-16 20:20 UTC #12 lore raw

In reply to Grant Likely

Quoted 12 lines · Grant Likely

Quoted 10 lines · Wolfgang Grandegger
Currently, the I2C buses are probed for HWMON I2C devices, which might not be acceptable in same cases. This patch makes device probing configurable through the property "probe" of the FDT I2C device node:
      i2c@3000 {
           ...
           compatible = "fsl-i2c";
           probe;
	    ...
       };

Quoted 4 lines

You need to add documentation to booting-without-of.txt about what the 'probe' property means. Also, 'probe' is a pretty generic term being used in an i2c specific context. Can you prefix it with i2c or something to protect the namespace? 'i2c-probe' perhaps?

Of course, no problem. I'm also aware of booting-without-of.txt but first I wanted to bring up the problem-

Quoted 6 lines

Quoted 4 lines
Assuming that systems using fsl-i2c should have proper platform data, probing is disabled by default. Unfortunately, that's not the case and various DTS files would require to be updated.
This comment doesn't seem to be correct.

Well, maybe it's due to my bad English. What I wanted to say is that by default probing should be disabled. But that requires various DTS files to be fixed.

Quoted 8 lines

Quoted 5 lines
---
arch/powerpc/sysdev/fsl_soc.c |    4 ++++
drivers/i2c/busses/i2c-mpc.c  |   17 ++++++++++++++---
include/linux/fsl_devices.h   |    1 +
This will need to be reworked for current top of tree. i2c-mpc dependence on fsl_soc.c has been removed.

I kown. Will prepare a new patch a.s.a.p.

Wolfgang.

Evangelion

2008-07-17 10:03 UTC #13 lore raw

Is there relationship between address translation enabled and PLB timeout error?

Hello, all:
I am building a Linux kernel module for PPC405EP. My developing
board is PPChameleonEVB. I am debugging with BDI2000 and GDB, and my problem is:
In GDB, a section of the codes is disassembled to:
mfmsr r0
ori r0,r0,32768
mtmsr r0
blr
From BDI2000, I have checked that after "ori", GPR0 contains
"0x00029030". This value should be written into MSR by "mtmsr" to set EE bit of MSR as 1, but after single step in BDI, "mtmsr" does not work as hoped. MSR becomes "0x00000030", GPR0 becomes some weird number, and there is "Step timeout detected". Meanwhile, the board traps into "Data machine check in kernel mode". I also have tried "wrteei 1" instead of the codes above, but failed again. However, those codes works well in PPC440EP Yosemite board.
I have compared PPChameleonEVB and Yosemite. It seems the most
significant difference is for PPChameleonEVB, address translation is enabled by default (MSR[IR, DR] = 1), while for Yosemite, it is disabled (MSR[IR, DR] = 0). And from the error message, I think there is a problem with PLB because PLB0_BESR becomes 0x00c00000 from 0x00000000 and PLB0_BEAR becomes 0x03066004 when machine check happens. Based on the processor manual, the PLB0_BESR shows the PLB Timeout Error Status and the value means the error operation is reading the processor core DCU. The PLB0_BEAR shows the address of the access on which a bus timeout error occurred. So I have the following questions for this moment:
1. Is it possible or not that address translation leads to the PLB
timeout error? If that is the cause, how to fix the problem?
2. Is the address in PLB0_BEAR a memory address (real address or
effective address) or a bus address (not an address in any kind of memory)?
3. Are there other reasons for the machine check in this situation?
4. Is it an unrecoverable hardware problem (bug) or not?

Here is the debugging log:

405EP>ti

    Core number       : 0
    Core state        : debug mode
    Debug entry cause : single step
    Current PC        : 0xc32b1008
    Current CR        : 0x84000084
    Current MSR       : 0x00021030
    Current LR        : 0xc32b46c4

405EP>rd
GPR00: 00029030 c1dd9d60 c1fe7bf0 00000000
GPR04: 00000001 00000000 c32b2c8c 00000000
GPR08: c3068000 c3068000 00000001 c3062000
GPR12: 00000000 10019dd8 c32c0000 c32b0000
GPR16: 00000001 c32b0000 00000002 7ff4f670
GPR20: 00000028 c32b0000 c32b0000 10011000
GPR24: c306a000 00000000 00000000 10012c6c
GPR28: c18e4000 c32c0000 00000000 00000000
CR : 84000084 MSR: 00021030
405EP>ti

    Core number       : 0
    Core state        : debug mode
    Debug entry cause : JTAG stop request
    Current PC        : 0xc000490c
    Current CR        : 0x42000082
    Current MSR       : 0x00000030
    Current LR        : 0xc001f1b8

# Step timeout detected
405EP>rd
GPR00: 03929800 c02f3e60 c3066000 000102f1
GPR04: 00005424 00000007 c0146f3c c0260000
GPR08: 00000000 c02d0000 c3062000 00000000
GPR12: 00000000 10019dd8 c32c0000 c32b0000
GPR16: 00000001 c32b0000 00000002 7ff4f670
GPR20: 00000028 c32b0000 c32b0000 10011000
GPR24: c306a000 00000000 00000000 10012c6c
GPR28: c02f0000 00000152 c3066000 c02f0000
CR : 42000082 MSR: 00000030

==========================================
Here is the error message:

Data machine check in kernel mode.
PLB0: BEAR= 0x03066004 ACR= 0x00000000 BESR= 0x00c00000
PLB0 to OPB: BEAR= 0x04000000 BESR0= 0x00000000 BESR1= 0x00000000

Oops: machine check, sig: 7 [#1]
NIP: 00002AD0 LR: 000005A0 CTR: C000CC58
REGS: c02f3f50 TRAP: 0202   Not tainted (2.6.19.2-eldk)
MSR: 00021000 <ME>  CR: 24000084  XER: 20000000

TASK = c3066000[0] '' THREAD: c02d2000
GPR00: 00029030 C1DD9CA0 C3066000 C1DD9CB0 00000001 00000000 C32B2C8C 00000000
GPR08: C3068000 00000000 00021032 01DD9CA0 030661B0 10019DD8 C32C0000 C32B0000
GPR16: 00000001 C32B0000 00000002 7FF4F670 00000028 C32B0000 C32B0000 10011000
GPR24: C306A000 00000000 00000000 10012C6C C18E4000 C32C0000 00000000 00000000
NIP [00002AD0] 0x2ad0
LR [000005A0] 0x5a0

Call Trace:
Instruction dump:
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
Kernel panic - not syncing: Attempted to kill the idle task!
 <0>Rebooting in 180 seconds..

Thanks a lot for advice!

Best Regards

Evangelion
July 17th, 2008

__________________________________________________
�Ͽ�ע��Ż��?
http://cn.mail.yahoo.com