[patch 0/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[patch 0/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Christian Krafft]

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

Re: [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Christian Krafft]

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.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>

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
--- linux-2.6.19-rc6.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.19-rc6/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
  *         source@mvista.com
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
  *
  *  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

Re: [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Heiko Joerg Schick]

Christian Krafft wrote:
> 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.
>
> Signed-off-by: Christian Krafft <krafft@de.ibm.com>
>   
Acked-by: Heiko J Schick <schihei@de.ibm.com>

Regards,
    Heiko

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Corey Minyard]

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

Christian Krafft wrote:
> +};
> +
> +static void __devinit of_find_bmc(void)
> +{
> +	of_register_platform_driver(&ipmi_of_platform_driver);
> +}
> +#endif /* CONFIG_PM_OF */
>   
I took the liberty of changing this to "CONFIG_PPC_OF"

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Christian Krafft]

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


On Fri, 08 Dec 2006 11:19:56 -0600
Corey Minyard <minyard@acm.org> wrote:

> Ok, patch is in my tree with some minor changes required due to other
> changes that have gone in since 2.6.19.
>=20
> Christian Krafft wrote:
> > +};
> > +
> > +static void __devinit of_find_bmc(void)
> > +{
> > +	of_register_platform_driver(&ipmi_of_platform_driver);
> > +}
> > +#endif /* CONFIG_PM_OF */
> >
> I took the liberty of changing this to "CONFIG_PPC_OF"
>=20


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

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Corey Minyard]

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.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>
Acked-by: Heiko J Schick <schihei@de.ibm.com>
Signed-off-by: Corey Minyard <minyard@acm.org>

Index: linux-2.6.19/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.19.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.19/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
  *         source@mvista.com
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
  *
  *  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);

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Friday 08 December 2006 19:59, Corey Minyard wrote:

> 
> +       if (match->data) {
> +               dev_warn(&dev->dev, "unknown interface type\n");
> +               return -ENODEV;
> +       }
> +
> 
> What's this mean?  I don't understand the error.

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

> +	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.

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.

> +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.

ok, great.

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

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.

> +static void __devinit of_find_bmc(void)
> +{
> +	of_register_platform_driver(&ipmi_of_platform_driver);
> +}
> +#endif /* CONFIG_PPC_OF */

I guess this function could also be removed entirely, it doesn't
add anything useful.

	Arnd <><

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Benjamin Herrenschmidt]

> 
> +	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.

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.

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Friday 08 December 2006 23:50, Benjamin Herrenschmidt wrote:
>=20
> >=20
> > +=A0=A0=A0=A0=A0info->io.regsize=A0=A0=A0=A0=A0=A0=A0=A0=3D resource0.e=
nd - resource0.start + 1;
> > +=A0=A0=A0=A0=A0info->io.regspacing=A0=A0=A0=A0=A0=3D resource1.start -=
 resource0.start;
> >=20
> > Are you sure this is a reliable way to check the register spacing and
> > register size? =A0Register size means "how big is a register (8, 16, 32
> > bits)". =A0Register spacing means (how many bytes are there between
> > registers. =A0If you had two registers that were 8 bits and 4 bytes
> > apart, for instance, I don't believe the above calculations would work.
>=20
> 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.

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 <><

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Corey Minyard]

Arnd Bergmann wrote:
> On Friday 08 December 2006 23:50, Benjamin Herrenschmidt wrote:
>   
>>> +     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.
>>>       
>> 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.
>>     
>
> 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.
>   
The BT and SMIC interfaces have three registers.  Does that break things?

-Corey

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Saturday 09 December 2006 01:07, Corey Minyard wrote:
> > 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.
> > =A0=20
> The BT and SMIC interfaces have three registers. =A0Does that break thing=
s?
>=20
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 <><

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Heiko J Schick]

Hello,

On 09.12.2006, at 12:44, Arnd Bergmann wrote:

> On Saturday 09 December 2006 01:07, Corey Minyard wrote:
>>> 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.
>>>
>> The BT and SMIC interfaces have three registers.  Does that break  
>> things?
>>
> 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).

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Sunday 10 December 2006 19:42, Heiko J Schick wrote:
> On 09.12.2006, at 12:44, Arnd Bergmann wrote:
> 
> 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.

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.

> 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.

yes

> 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).

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 <><

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Heiko J Schick]

Hello,

On Dec 11, 2006, at 11:11 AM, Arnd Bergmann wrote:

> 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.

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Segher Boessenkool]

>> 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.
>
> 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.

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.

> 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.

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Segher Boessenkool]

> 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.

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.

> (2)
> name = ipmi
> device_type

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).

> 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.

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.

> Because of that
> you have to include additional properties.

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.

> 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.

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.

> Personally I prefer (1), because I a the moment I don't seea big
> benefit in (2). For both options of have to implement source code

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

> 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.

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Monday 11 December 2006 14:18, Segher Boessenkool wrote:
> > 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.
>=20
> 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.

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.

> > (2)
> > name =3D ipmi
> > device_type
>=20
> device_type =3D ipmi-kcs
>=20
> 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).

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.

> > regs =3D <addr> <len>
> > reg-spacing =3D <number>
> > reg-size =3D <number>
> > reg-shift =3D <number>
> >
> > The main difference between (1) and (2) is that (2) will represent
> > the whole register region within the reg property.
>=20
> Which is the proper thing to do. =A0The "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.

ok

	Arnd <><

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Heiko J Schick]

Hello,

On Dec 11, 2006, at 3:25 PM, Arnd Bergmann wrote:

> device-type = ipmi
> compatible = ipmi-kcs

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Segher Boessenkool]

> 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

Yeah.

> reg-size    = 1
> reg-shift   = 0

But without these two, they are the default.

> Is the "8" of the reg property correct?

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'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?

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Christian Krafft]

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

Cheers, ck

On Mon, 11 Dec 2006 17:58:36 +0100
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > If I understood you comments an example OFDT node for IPMI could
> > look like the following:
> >
> > name =3D "ipmi"
> > device_type =3D "ipmi"
> > compatible =3D "ipmi-kcs"
> >
> > reg =3D XXXXXXX8 00000008
> > reg-spacing =3D 4
>=20
> Yeah.
>=20
> > reg-size    =3D 1
> > reg-shift   =3D 0
>=20
> But without these two, they are the default.
>=20
> > Is the "8" of the reg property correct?
>=20
> 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.
>=20
> > 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?
>=20
> 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!)
>=20
>=20
> Segher
>=20


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

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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Segher Boessenkool]

> Why not this?
>
> device-type = ipmi
> compatible = ipmi-kcs

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

> 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=pci and compatible=whatever,
> which tells you how to access the config space registers.

"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

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Benjamin Herrenschmidt]

On Sat, 2006-12-09 at 01:00 +0100, Arnd Bergmann wrote:
> On Friday 08 December 2006 23:50, Benjamin Herrenschmidt wrote:
> > 
> > > 
> > > +     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.
> > 
> > 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.
> 
> 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.

For only 2 registers, it makes sense yes.

Ben.

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Segher Boessenkool]

>> 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.

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.

> I think the current representation is perfect. AFAICS, there are  
> always
> two registers,

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.

> but depending on the HW implementation, they may be
> between 1 and 4 bytes wide, and can have a different spacing.

Always one byte wide.

> 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.

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


Segher

Re: [Openipmi-developer] [patch 1/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Saturday 09 December 2006 10:46, Segher Boessenkool wrote:
> How do you know? =A0Some later compatible implementation might add
> some regs. =A0Also, there are implementations that simply ignore
> the lower address bits, so size=3D1 in the "reg" property is wrong
> for those.
>=20
> > but depending on the HW implementation, they may be
> > between 1 and 4 bytes wide, and can have a different spacing.
>=20
> Always one byte wide.

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 <><

Re: [patch 0/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Paul Mackerras]

Christian Krafft writes:

> 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 without
> specifying the resources as module parameters.

Which powerpc-based machines have IPMI?

Paul.

Re: [patch 0/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Arnd Bergmann]

On Friday 08 December 2006 01:41, Paul Mackerras wrote:
> Christian Krafft writes:
> 
> > 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 without
> > specifying the resources as module parameters.
> 
> Which powerpc-based machines have IPMI?

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 <><

Re: [Openipmi-developer] [patch 0/1] ipmi: add autosensing of ipmi device on powerpc using device-tree

[Corey Minyard]

Arnd Bergmann wrote:
> On Friday 08 December 2006 01:41, Paul Mackerras wrote:
>   
>> Christian Krafft writes:
>>
>>     
>>> 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 without
>>> specifying the resources as module parameters.
>>>       
>> Which powerpc-based machines have IPMI?
>>     
>
> 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.
>   
Also, all ATCA  powerpc boards have IPMI.

-Corey