| Message ID | 1462995966-1184-8-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 --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); }