Re: [Qemu-devel] [PATCH 2/3] target-i386: reserve RCRB mmio space in ACPI DSDT table

[Paulo Alcantara <pcacjr@zytor.com>]

On Wed, 27 May 2015 14:03:59 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 27/05/2015 02:29, Paulo Alcantara wrote:
> > Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> > ---
> >  hw/i386/acpi-dsdt-pdrc.dsl    |  46
> > ++++++++++++++++++++++++++++++++++++++++++
> 
> Why pdrc and not e.g. ccr (chipset configuration registers)?  I cannot
> find PDRC / PCI device resource consumption in the ICH9 spec.

I've found the "PDRC" name being used in ACPI DSDT table definitions on
the internet for the MS legacy PNP ID "PNP0C02". However, the ICH9 does
mention "Chipset Configuration registers" so it's really more
meaningful to name it as "CCR" and then matching spec.

> 
> >  hw/i386/q35-acpi-dsdt.dsl     |   1 +
> >  tests/acpi-test-data/q35/DSDT | Bin 7666 -> 7795 bytes
> >  3 files changed, 47 insertions(+)
> >  create mode 100644 hw/i386/acpi-dsdt-pdrc.dsl
> > 
> > diff --git a/hw/i386/acpi-dsdt-pdrc.dsl b/hw/i386/acpi-dsdt-pdrc.dsl
> > new file mode 100644
> > index 0000000..badb410
> > --- /dev/null
> > +++ b/hw/i386/acpi-dsdt-pdrc.dsl
> > @@ -0,0 +1,46 @@
> > +/*
> > + * 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 Free Software Foundation; either version 2 of the License,
> > or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public
> > License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/****************************************************************
> > + * PCI Device Resource Comsumption
> 
> "Chipset configuration registers"

OK.

> 
> > + ****************************************************************/
> > +
> > +Scope(\_SB.PCI0) {
> > +    Device (PDRC) {
> 
> Device (CCR)

OK.

> 
> > +        Name (_HID, EISAID("PNP0C02"))
> > +        Name (_UID, 1)
> > +
> > +        Name (PDRS, ResourceTemplate() {
> 
> Just use Name(_CRS, ResourceTemplate() { ... })

OK.

> 
> > +	    Memory32Fixed(ReadWrite, 0xfed1c000, 0x00004000)
> > +        })
> > +
> > +        Method (_CRS, 0, Serialized) {
> > +            Return(PDRS)
> > +        }
> > +    }
> > +}
> > +
> > +Scope(\_SB) {
> > +    OperationRegion (RCRB, SystemMemory, 0xfed1c000, 0x4000)
> > +    Field (RCRB, DWordAcc, Lock, Preserve) {
> > +        Offset(0x3000),
> > +	TCTL, 8,
> > +	, 24,
> > +	Offset(0x3400),
> > +	RTCC, 32,
> > +	HPTC, 32,
> > +	GCSR, 32,
> > +    }
> 
> Why do you need the RCRB OperationRegion if you never access it?

Indeed. It's not being referenced in any AML code so I'll remove it.

> 
> > +}
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 16eaca3..32b680e 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -114,6 +114,7 @@ DefinitionBlock (
> >          }
> >      }
> >  
> > +#include "acpi-dsdt-pdrc.dsl"
> 
> Just include it in this file since it's not shared between i440FX and
> Q35.

OK.

I'll send a v2 with all proposed changes.

Thanks,

Paulo 

-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.