ipmi: add autosensing of ipmi device on powerpc using device-tree

Hi openipmi-developers,

the patch I'll send, adds of_device support to the ipmi_si module. Having the correct device-tree entries, the ipmi module can be loaded witho= ut
specifying the resources as module parameters.

If it's looking okay, it would be nice, if you can merge this one.

--=20
Mit freundlichen Gr=FCssen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist

This patch adds support for of_platform_driver to the ipmi_si module. When loading the module, the driver will be registered to of_platform. The driver will be probed for all devices with the type ipmi. It's supporti= ng
devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt. Only ipmi-kcs could be tested.

Index: linux-2.6.19-rc6/drivers/char/ipmi/ipmi_si_intf.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

@@ -9,6 +9,7 @@
  *         [email protected]
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <[email protected]>
  *
  *  This program is free software; you can redistribute it and/or modify it
  *  under the terms of the GNU General Public License as published by the

@@ -62,6 +63,12 @@
 #include <linux/init.h>
 #include <linux/dmi.h>

=20
+/* only exists on powerpc */
+#ifdef CONFIG_PPC_OF
+#include <asm/of_device.h>
+#include <asm/of_platform.h>
+#endif
+
/* Measure times between events in the driver. */
#undef DEBUG_TIMING
=20
@@ -1884,6 +1891,90 @@ static struct pci_driver ipmi_pci_driver
#endif /* CONFIG_PCI */
=20
=20
+#ifdef CONFIG_PPC_OF
+static int ipmi_of_probe(struct of_device *dev, const struct of_device_id = *match)
+{
+ struct smi_info *info;
+ struct resource resource0, resource1;
+ struct device_node *np =3D dev->node;
+ int ret;
+
+ dev_info(&dev->dev, "probing via device tree\n");
+
+ if (match->data) {
+ dev_warn(&dev->dev, "unknown interface type\n");
+ return -ENODEV;
+ }
+
+ ret =3D of_address_to_resource(np, 0, &resource0);
+
+ if (ret) {
+ dev_warn(&dev->dev, "invalid address\n");
+ return ret;
+ }
+
+ ret =3D of_address_to_resource(np, 1, &resource1);
+
+ if (ret) {
+ dev_warn(&dev->dev, "invalid address\n");
+ return ret;
+ }
+
+ info =3D kzalloc(sizeof(*info), GFP_KERNEL);
+
+ if (!info) {
+ dev_err(&dev->dev, "could not allocate memory\n");
+ return -ENOMEM;
+ }
+
+ info->si_type =3D (enum si_type) match->data;
+ info->addr_source =3D "device-tree";
+ info->io_setup =3D mem_setup;
+ info->irq_setup =3D std_irq_setup;
+
+ info->io.addr_type =3D IPMI_MEM_ADDR_SPACE;
+ info->io.addr_data =3D resource0.start;
+ info->io.regsize =3D resource0.end - resource0.start + 1;
+ info->io.regspacing =3D resource1.start - resource0.start;
+ info->io.regshift =3D 0; /* regshift property if needed */
+ info->irq =3D irq_of_parse_and_map(dev->node, 0);
+ info->dev =3D &dev->dev;
+
+ dev_dbg(&dev->dev, "addr 0x%lx regsize %ld spacing %ld irq %x\n",
+ info->io.addr_data, info->io.regsize, info->io.regspacing,
+ info->irq);
+
+ return try_smi_init(info);
+}
+
+static int ipmi_of_remove(struct of_device *dev)
+{
+ return 0;
+}
+
+static struct of_device_id ipmi_match[] =3D
+{
+ { .type =3D "ipmi", .compatible =3D "ipmi-kcs", .data =3D (void*)SI_KCS },
+ { .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =3D (void*)SI_SMIC=
},
+ { .type =3D "ipmi", .compatible =3D "ipmi-bt", .data =3D (void*)SI_BT },
+ {},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =3D
+{
+ .name =3D "ipmi",
+ .match_table =3D ipmi_match,
+ .probe =3D ipmi_of_probe,
+ .remove =3D ipmi_of_remove
+};
+
+static void __devinit of_find_bmc(void)
+{
+ of_register_platform_driver(&ipmi_of_platform_driver);
+}
+#endif /* CONFIG_PM_OF */
+
+
static int try_get_dev_id(struct smi_info *smi_info)
{
unsigned char msg[2];
@@ -2486,6 +2577,10 @@ static __devinit int init_ipmi_si(void)
pci_module_init(&ipmi_pci_driver);
#endif
=20
+#ifdef CONFIG_PPC_OF
+ of_find_bmc();
+#endif
+
if (si_trydefaults) {
mutex_lock(&smi_infos_lock);
if (list_empty(&smi_infos)) {
@@ -2583,6 +2678,10 @@ static __exit void cleanup_ipmi_si(void)
pci_unregister_driver(&ipmi_pci_driver);
#endif
=20
+#ifdef CONFIG_OF_PPC
+ of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+
mutex_lock(&smi_infos_lock);
list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
cleanup_one_si(e);

--=20
Mit freundlichen Gr=FCssen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist

Regards,
Heiko

Ok, patch is in my tree with some minor changes required due to other changes that have gone in since 2.6.19.

sorry and thanks,
I'll send an updated version with the comments about regsize worked in.

--=20
Mit freundlichen Gr=FCssen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist

I reviewed this some more, and I cleaned up some formatting (breaking long lines) and enhanced the error reporting strings.

I believe the following:

@@ -2583,6 +2678,10 @@ static __exit void cleanup_ipmi_si(void)
pci_unregister_driver(&ipmi_pci_driver);
#endif

+#ifdef CONFIG_OF_PPC
+ of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+

Uses the wrong CONFIG option, which I have adjusted.

+ if (match->data) {
+ dev_warn(&dev->dev, "unknown interface type\n");
+ return -ENODEV;
+ }
+

What's this mean? I don't understand the error.

+ info->io.regsize = resource0.end - resource0.start + 1;
+ info->io.regspacing = resource1.start - resource0.start;

Are you sure this is a reliable way to check the register spacing and register size? Register size means "how big is a register (8, 16, 32 bits)". Register spacing means (how many bytes are there between registers. If you had two registers that were 8 bits and 4 bytes apart, for instance, I don't believe the above calculations would work.

+static int ipmi_of_remove(struct of_device *dev)
+{
+ return 0;
+}

With the newest IPMI patches (only in git right now), hot removal of interfaces is supported. Is that what this is for? If so, you can call cleanup_one_si() on the interface.

Here's a new patch. Can you test that unloading the module works correctly?

-Corey

This patch adds support for of_platform_driver to the ipmi_si module. When loading the module, the driver will be registered to of_platform. The driver will be probed for all devices with the type ipmi. It's supporting devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt. Only ipmi-kcs could be tested.

Index: linux-2.6.19/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
@@ -9,6 +9,7 @@
  *         [email protected]
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <[email protected]>
  *
  *  This program is free software; you can redistribute it and/or modify it
  *  under the terms of the GNU General Public License as published by the

@@ -64,6 +65,12 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 
+/* only exists on powerpc */
+#ifdef CONFIG_PPC_OF
+#include <asm/of_device.h>
+#include <asm/of_platform.h>
+#endif
+
 #define PFX "ipmi_si: "
 
 /* Measure times between events in the driver. */

@@ -2174,6 +2181,89 @@ static struct pci_driver ipmi_pci_driver
 #endif /* CONFIG_PCI */
 
 
+#ifdef CONFIG_PPC_OF
+static int ipmi_of_probe(struct of_device *dev,
+			 const struct of_device_id *match)
+{
+	struct smi_info *info;
+	struct resource resource0, resource1;
+	struct device_node *np = dev->node;
+	int ret;
+
+	dev_info(&dev->dev, PFX "probing via device tree\n");
+
+	if (match->data) {
+		dev_warn(&dev->dev, PFX "unknown OF interface type\n");
+		return -ENODEV;
+	}
+
+	ret = of_address_to_resource(np, 0, &resource0);
+	if (ret) {
+		dev_warn(&dev->dev, PFX "invalid address 0 from OF\n");
+		return ret;
+	}
+
+	ret = of_address_to_resource(np, 1, &resource1);
+	if (ret) {
+		dev_warn(&dev->dev, PFX "invalid address 1 from OF\n");
+		return ret;
+	}
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&dev->dev,
+			PFX "could not allocate memory for OF probe\n");
+		return -ENOMEM;
+	}
+
+	info->si_type		= (enum si_type) match->data;
+	info->addr_source	= "device-tree";
+	info->io_setup		= mem_setup;
+	info->irq_setup		= std_irq_setup;
+
+	info->io.addr_type	= IPMI_MEM_ADDR_SPACE;
+	info->io.addr_data	= resource0.start;
+	info->io.regsize	= resource0.end - resource0.start + 1;
+	info->io.regspacing	= resource1.start - resource0.start;
+	info->io.regshift	= 0; /* regshift property if needed */
+	info->irq		= irq_of_parse_and_map(dev->node, 0);
+	info->dev		= &dev->dev;
+
+	dev_dbg(&dev->dev, "addr 0x%lx regsize %ld spacing %ld irq %x\n",
+		info->io.addr_data, info->io.regsize, info->io.regspacing,
+		info->irq);
+
+	return try_smi_init(info);
+}
+
+static int ipmi_of_remove(struct of_device *dev)
+{
+	return 0;
+}
+
+static struct of_device_id ipmi_match[] =
+{
+	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data = (void*)SI_KCS },
+	{ .type = "ipmi", .compatible = "ipmi-smic", .data = (void*)SI_SMIC },
+	{ .type = "ipmi", .compatible = "ipmi-bt",   .data = (void*)SI_BT },
+	{},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =
+{
+	.name		= "ipmi",
+	.match_table	= ipmi_match,
+	.probe		= ipmi_of_probe,
+	.remove		= ipmi_of_remove
+};
+
+static void __devinit of_find_bmc(void)
+{
+	of_register_platform_driver(&ipmi_of_platform_driver);
+}
+#endif /* CONFIG_PPC_OF */
+
+
 static int try_get_dev_id(struct smi_info *smi_info)
 {
 	unsigned char         msg[2];

@@ -2798,6 +2888,10 @@ static __devinit int init_ipmi_si(void)
 	}
 #endif
 
+#ifdef CONFIG_PPC_OF
+	of_find_bmc();
+#endif
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {

@@ -2895,6 +2989,10 @@ static __exit void cleanup_ipmi_si(void)
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
 
+#ifdef CONFIG_PPC_OF
+	of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	mutex_lock(&smi_infos_lock);
 	list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
 		cleanup_one_si(e);

The check is bogus and should be removed. The effect is that only SI_KCS can work, all others will be rejected.

Looks right to me. Assume these of properties

#address-cells: <1>
#size-cells: <1>
reg: <0x100> <1> <0x104> <1> /* start, length, start, length */

converting them to resources gives you

resource0 = { .start = 0x100, .end = 0x100, }
resource1 = { .start = 0x104, .end = 0x104, }

consequently, you end up with

info->io = {
	.regsize = 1,
	.regspacing = 4,
	.addr_data = 0x100,
};

which is exactly what you want.

ok, great.

Christian has already gone home for the weekend, and I don't have access to his machine, so testing will probably have to wait until Monday.

	Arnd <><

How many registers do we expect here ? Might be better to have one resource represent the whole MMIO area, and have a separate property that indicates the stride between 2 registers.

Ben.

nd - resource0.start + 1;

resource0.start;

I think the current representation is perfect. AFAICS, there are always two registers, but depending on the HW implementation, they may be between 1 and 4 bytes wide, and can have a different spacing.

By having two separate areas in the reg property, the driver can easily determine both the size and the spacing. It will then do a single ioremap that spans both anyway.

	Arnd <><

The BT and SMIC interfaces have three registers. Does that break things?

-Corey

s?

It should still work, but I have to admit that at this point it gets ugly.

How about defining the properties just like the driver expects them:

reg <size> <len>: the area spanning all registers (2 or three)
reg-spacing <number>: offset of the start of each register (default 1)
reg-size <number>: length of each register (1, 2, or 4, default 1)
reg-shift <number>: bit-position of the data inside the register

			(default 0).

	Arnd <><

Hello,

We have to include the IPMI information in a way which can be used for controllers which are accessed via memory mapped or legacy I/O. Furthermore, we have to include the register information that it will work not only for KCS interfaces. It should be possible to include BT and SMIC interfaces, too.

We have two options how we wanna present IPMI in the OFDT.

(1)

name = ipmi
device_type = ipmi
compatible = ipmi-kcs, ipmi-1.5, ...

regs = <addr> <len> <addr> <len> (for KCS interfaces)
or
regs = <addr> <len> <addr> <len> <addr> <len> (for BT and SMIC interfaces)

(2)
name = ipmi
device_type
regs = <addr> <len>
reg-spacing = <number>
reg-size = <number>
reg-shift = <number>

The main difference between (1) and (2) is that (2) will represent the whole register region within the reg property. Because of that you have to include additional properties.

In (1) you will include every register (e.g. Cmd / Status, Data_In / Data_Out, etc.) within the reg property. The values for reg-spacing are calculated by subtracting addr2 - addr1. The reg-size are the len's within the reg property.

Personally I prefer (1), because I a the moment I don't see a big benefit in (2). For both options of have to implement source code which checks the compatible node to get the used interface type. According to the emitted interface type you have to read in (1) two or three registers from the reg property.

In (2) you have to calculate two or three addresses (with the help of reg and the additional reg-spacing, etc. properties).

Regards,
Heiko

There should be no legacy I/O on an of_platform device, but only on behind a PCI or ISA bus, right? For PCI, the probing happens using the pci ipmi driver, so that's not our problem here, and I hope we don't need to worry about ISA buses any more.

yes

I'd like to see (2), because that's what the driver currently expects. The common ipmi code in linux _always_ does this calculation anyway, and
(1) only means we have to compute the reg-spacing and reg-size from the layout of the registers, which gets rather complicated if you have more than two of them in the reg property.

	Arnd <><

Hello,

Yes, that is correct, but what happens if you have three registers and because of stupid reasons the reg-spacing and reg-size is different.

In (1) you can handle it without problems, in (2) not. Also, where is the difference if you have to calculate the reg-spacing, etc. (1) or the addresses (2). The effort is more or less exactly the same.

Regards,
Heiko

You mean, say, a 32-bit reg at (local) address 0 and 4, and an 8-bit reg at offset 16? Neither (1) nor (2) can handle that -- the Linux interface isn't generic enough for that. Both (1) and (2) can describe the register space just fine in "reg"; the "reg" property doesn't describe how to use the registers, just where they are.

I doubt the IPMI spec allows such weird devices anyway.

The device tree describes the hardware. How an OS (like Linux) has to interpret it to fit that info to its internal interfaces is the OS' problem, it's a big layering violation to put that info into the device tree.

I wouldn't worry about supporting non-KCS controllers in your Linux code right now at all -- you can't test it anyway, it's dead code until someone makes such a device.

Segher

How to access registers is implicit in the device tree; just look at the parent bus, it knows how to do it, given the "reg" value in your device.

device_type = ipmi-kcs

There's no real reason to include the IPMI version number in here, if you really need the version it's trivial to probe for; it's not a property of the hardware but of the software on the controller (unless some day the IPMI spec breaks backwards compatibility, of course).

Which is the proper thing to do. The "reg" property should describe all of the address space your device listens to. Some IPMI-KCS devices listen to just the two registers, some listen to an 8-reg block, some to a 16-reg block.

Yes, since the device driver needs some additional information. This is quite natural and logical. Trying to do some clever workaround will only come back to bite you later.

This is giving additional semantics to the "reg" property that is compatible to its standard semantics for "standard" IPMI-KCS devices (which don't really _need_ that extra info, those are the default values); however, they will not be compatible for some (future) "special" devices.

You shouldn't be thinking "how does Linux use the device tree"; you should think "how does the device tree describe the hardware".

The "reg" property has some semantics independent of the device node it is in, too -- only dependent on that device's parent bus. Don't break that :-)

Segher

right, though it's a little different if you need special opcodes, like DCR on ppc4xx or PIO on i386. Let's not worry about those for now.

Why not this?

device-type =3D ipmi
compatible =3D ipmi-kcs

I thought the device-type tells you what kind of bindings to use, and the compatible (and/or model) tells you how to
access it. E.g. all PHBs have device-type=3Dpci and compatible=3Dwhatever, which tells you how to access the config space registers.

ok

	Arnd <><

Hello,

If I understood you comments an example OFDT node for IPMI could look like the following:

name = "ipmi"
device_type = "ipmi"
compatible = "ipmi-kcs"

reg = XXXXXXX8 00000008
reg-spacing = 4
reg-size = 1
reg-shift = 0

In this example the Data_In / Data_out sitting at address XXXXXXX8 and Status / Command is sitting at XXXXXXXC. Does this layout look good to everybody?

Is the "8" of the reg property correct?

I'm still worried what happens when we have a BT or SMIC interface an the registers sitting at XXXXXXX4, XXXXXXX6, XXXXXXXA. In this case the reg-spacing property can't be used anymore. Can this happen in reality?

Regards,
Heiko

Yeah.

But without these two, they are the default.

Sure. Alternatively, if your device really only claims bus cycles targeting the 8-bit regs at xxx8 and xxxC, you can say that in the "reg" property. This doesn't change the programming model in the least though.

I don't think it is allowed. Either way, the generic Linux IPMI code would need changing for this -- let's worry about it later (if ever!)

Segher

Good then, it make things easier for me ;-)
Will send an updated version soon...

Cheers, ck

--=20
Mit freundlichen Gr=FCssen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist

That's fine too. Better even, the more I think about it.

"device_type" (note the spelling -- historical accident) shows the generic type of the device -- the programming model for it (for OF, not for the OS necessarily!).

"model" is just the exact device type, for identification/ diagnostic purposes primarily.

"compatible" is a list of identification strings the OS should use to find a device driver to drive this thing.

Segher

For only 2 registers, it makes sense yes.

Ben.

Yes *please*. Anything else will cause no end of problems, and that only for wanting to not create a perfectly reasonable extra property, with obvious semantics.

How do you know? Some later compatible implementation might add some regs. Also, there are implementations that simply ignore the lower address bits, so size=1 in the "reg" property is wrong for those.

Always one byte wide.

It's much better to make it explicit and obvious in the device tree what's going on.

Segher

AFAICS, some implementations require word access on the registers, even if only part of it is used. That's why the driver has the concept of reg-size and reg-shift.

	Arnd <><

Which powerpc-based machines have IPMI?

Paul.

This particular one is for an upcoming product, benh knows which one this is for. The JS21 blade already has an IPMI controller, though that is hidden behind the hypervisor, see
http://www-03.ibm.com/systems/bladecenter/js21/specs.html.

	Arnd <><

Also, all ATCA powerpc boards have IPMI.

-Corey