diff mbox

[7/7] ipmi: Add ACPI to the SMBus IPMI device

Message ID 1462995966-1184-8-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard May 11, 2016, 7:46 p.m. UTC 
From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/ipmi/smbus_ipmi.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael S. Tsirkin May 12, 2016, 7:36 a.m. UTC | #1 
On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  hw/ipmi/smbus_ipmi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> index 4e7203b..3a34aaf 100644
> --- a/hw/ipmi/smbus_ipmi.c
> +++ b/hw/ipmi/smbus_ipmi.c
> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>      sid->fwinfo.base_address = sid->parent.i2c.address;
>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>      sid->fwinfo.register_spacing = 1;
> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";

I don't think it's a good idea to spread things like PCI0
outside acpi-build.c. Why do you want to pass in the path
at all?

>      ipmi_add_fwinfo(&sid->fwinfo, errp);
>  }
>  
> -- 
> 2.7.4
Corey Minyard May 12, 2016, 1:32 p.m. UTC | #2 
On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>>   hw/ipmi/smbus_ipmi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
>> index 4e7203b..3a34aaf 100644
>> --- a/hw/ipmi/smbus_ipmi.c
>> +++ b/hw/ipmi/smbus_ipmi.c
>> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>>       sid->fwinfo.base_address = sid->parent.i2c.address;
>>       sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>>       sid->fwinfo.register_spacing = 1;
>> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> I don't think it's a good idea to spread things like PCI0
> outside acpi-build.c. Why do you want to pass in the path
> at all?

You have to define the namespace for the ASL definition,
and you have to give the name in the serial bus definition.
However, thinking it through some more, this name needs
to come from the I2C device, not just hard-coded here.

-corey

>>       ipmi_add_fwinfo(&sid->fwinfo, errp);
>>   }
>>   
>> -- 
>> 2.7.4
Michael S. Tsirkin May 12, 2016, 1:35 p.m. UTC | #3 
On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> >On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> >>From: Corey Minyard <cminyard@mvista.com>
> >>
> >>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>---
> >>  hw/ipmi/smbus_ipmi.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >>diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> >>index 4e7203b..3a34aaf 100644
> >>--- a/hw/ipmi/smbus_ipmi.c
> >>+++ b/hw/ipmi/smbus_ipmi.c
> >>@@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
> >>      sid->fwinfo.base_address = sid->parent.i2c.address;
> >>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
> >>      sid->fwinfo.register_spacing = 1;
> >>+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> >I don't think it's a good idea to spread things like PCI0
> >outside acpi-build.c. Why do you want to pass in the path
> >at all?
> 
> You have to define the namespace for the ASL definition,
> and you have to give the name in the serial bus definition.
> However, thinking it through some more, this name needs
> to come from the I2C device, not just hard-coded here.
> 
> -corey

For now you can put it as PCI0 within aml-build.c.

Also do we need \\_SB.PCI0? Why not just create the
device within PCI0 scope?

> >>      ipmi_add_fwinfo(&sid->fwinfo, errp);
> >>  }
> >>-- 
> >>2.7.4
Corey Minyard May 12, 2016, 7:20 p.m. UTC | #4 
On 05/12/2016 08:35 AM, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
>> On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
>>> On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>   hw/ipmi/smbus_ipmi.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
>>>> index 4e7203b..3a34aaf 100644
>>>> --- a/hw/ipmi/smbus_ipmi.c
>>>> +++ b/hw/ipmi/smbus_ipmi.c
>>>> @@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
>>>>       sid->fwinfo.base_address = sid->parent.i2c.address;
>>>>       sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
>>>>       sid->fwinfo.register_spacing = 1;
>>>> +    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
>>> I don't think it's a good idea to spread things like PCI0
>>> outside acpi-build.c. Why do you want to pass in the path
>>> at all?
>> You have to define the namespace for the ASL definition,
>> and you have to give the name in the serial bus definition.
>> However, thinking it through some more, this name needs
>> to come from the I2C device, not just hard-coded here.
>>
>> -corey
> For now you can put it as PCI0 within aml-build.c.
>
> Also do we need \\_SB.PCI0? Why not just create the
> device within PCI0 scope?
I'm not sure I follow here. \SB.PCI0.SMB0 is the proper
scope to identify which SMBus the device is on.  This
is how the ISA devices are added, for instance.

What I've done now to fix this is added an ACPI namespace
to the I2C bus structure and stored the value for the I2C
bus there in the i386 code.

Maybe it should be in BusState?  That way the ISA IPMI code
could pull it from the bus, too.

Putting PCI0 into aml-build.c (or hw/acpi/ipmi.c, really) seems
like a violation of scope.

-corey

>>>>       ipmi_add_fwinfo(&sid->fwinfo, errp);
>>>>   }
>>>> -- 
>>>> 2.7.4
Michael S. Tsirkin May 12, 2016, 7:34 p.m. UTC | #5 
On Thu, May 12, 2016 at 02:20:25PM -0500, Corey Minyard wrote:
> On 05/12/2016 08:35 AM, Michael S. Tsirkin wrote:
> >On Thu, May 12, 2016 at 08:32:51AM -0500, Corey Minyard wrote:
> >>On 05/12/2016 02:36 AM, Michael S. Tsirkin wrote:
> >>>On Wed, May 11, 2016 at 02:46:06PM -0500, minyard@acm.org wrote:
> >>>>From: Corey Minyard <cminyard@mvista.com>
> >>>>
> >>>>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>>>---
> >>>>  hw/ipmi/smbus_ipmi.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>>diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
> >>>>index 4e7203b..3a34aaf 100644
> >>>>--- a/hw/ipmi/smbus_ipmi.c
> >>>>+++ b/hw/ipmi/smbus_ipmi.c
> >>>>@@ -167,6 +167,7 @@ static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
> >>>>      sid->fwinfo.base_address = sid->parent.i2c.address;
> >>>>      sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
> >>>>      sid->fwinfo.register_spacing = 1;
> >>>>+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
> >>>I don't think it's a good idea to spread things like PCI0
> >>>outside acpi-build.c. Why do you want to pass in the path
> >>>at all?
> >>You have to define the namespace for the ASL definition,
> >>and you have to give the name in the serial bus definition.
> >>However, thinking it through some more, this name needs
> >>to come from the I2C device, not just hard-coded here.
> >>
> >>-corey
> >For now you can put it as PCI0 within aml-build.c.
> >
> >Also do we need \\_SB.PCI0? Why not just create the
> >device within PCI0 scope?
> I'm not sure I follow here. \SB.PCI0.SMB0 is the proper
> scope to identify which SMBus the device is on.  This
> is how the ISA devices are added, for instance.

We add most of them within PCI0 scope instead.
E.g.

Device (PCI0) {
	Device (SMB0) {
	}
}

this way device does not need to know where it is.


> What I've done now to fix this is added an ACPI namespace
> to the I2C bus structure and stored the value for the I2C
> bus there in the i386 code.
> 
> Maybe it should be in BusState?  That way the ISA IPMI code
> could pull it from the bus, too.
> 
> Putting PCI0 into aml-build.c (or hw/acpi/ipmi.c, really) seems
> like a violation of scope.
> 
> -corey

Generally we scan each bus and list devices found there.


> >>>>      ipmi_add_fwinfo(&sid->fwinfo, errp);
> >>>>  }
> >>>>-- 
> >>>>2.7.4
diff mbox

Patch

diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
index 4e7203b..3a34aaf 100644
--- a/hw/ipmi/smbus_ipmi.c
+++ b/hw/ipmi/smbus_ipmi.c
@@ -167,6 +167,7 @@  static void smbus_ipmi_realize(DeviceState *dev, Error **errp)
     sid->fwinfo.base_address = sid->parent.i2c.address;
     sid->fwinfo.memspace = IPMI_MEMSPACE_SMBUS;
     sid->fwinfo.register_spacing = 1;
+    sid->fwinfo.acpi_parent = "\\_SB.PCI0.SMB0";
     ipmi_add_fwinfo(&sid->fwinfo, errp);
 }