Re: [Qemu-devel] [PATCH v7 3/3] ich9: implement strap SPKR pin logic

[Paulo Alcantara]

On Sun, 28 Jun 2015 10:37:58 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Sat, Jun 27, 2015 at 02:56:33PM -0300, Paulo Alcantara wrote:
> > If the signal is sampled high, this indicates that the system is
> > strapped to the "No Reboot" mode (ICH9 will disable the TCO Timer
> > system reboot feature). The status of this strap is readable via
> > the NO_REBOOT bit (CC: offset 0x3410:bit 5).
> > 
> > The NO_REBOOT bit is set when SPKR pin on ICH9 is sampled high.
> > This bit may be set or cleared by software if the strap is sampled
> > low but may not override the strap when it indicates "No Reboot".
> > 
> > This patch implements the logic where hardware has ability to set
> > SPKR pin through a property named "pin-spkr"
> 
> I would call it "noreboot" and not pin-spkr
> since that's what it does in the end.

Right. That's also more user intuitive, indeed.

> 
> > and it's sampled low by default.
> 
> I think sample high is a safer default.

OK. I'll default it to high.

> 
> > 
> > Signed-off-by: Paulo Alcantara <address@hidden>
> > ---
> >  hw/acpi/tco.c          |  3 ++-
> >  hw/isa/lpc_ich9.c      | 38 ++++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/ich9.h | 11 +++++++++++
> >  3 files changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
> > index 1794a54..c1f5739 100644
> > --- a/hw/acpi/tco.c
> > +++ b/hw/acpi/tco.c
> > @@ -64,7 +64,8 @@ static void tco_timer_expired(void *opaque)
> >          tr->tco.sts2 |= TCO_BOOT_STS;
> >          tr->timeouts_no = 0;
> >  
> > -        if (!(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> > +        if ((lpc->pin_strap.spkr & ICH9_PS_SPKR_PIN_LOW) &&
> > +            !(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> >              watchdog_perform_action();
> >              tco_timer_stop(tr);
> >              return;
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index b547002..49d1f30 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -575,11 +575,49 @@ static void ich9_lpc_get_sci_int(Object *obj,
> > Visitor *v, visit_type_uint32(v, &value, name, errp);
> >  }
> >  
> > +static void ich9_lpc_get_spkr_pin(Object *obj, Visitor *v,
> > +                                  void *opaque, const char *name,
> > +                                  Error **errp)
> > +{
> > +    ICH9LPCState *lpc = opaque;
> > +    uint8_t value = lpc->pin_strap.spkr;
> > +
> > +    visit_type_uint8(v, &value, name, errp);
> > +}
> > +
> > +static void ich9_lpc_set_spkr_pin(Object *obj, Visitor *v,
> > +                                  void *opaque, const char *name,
> > +                                  Error **errp)
> > +{
> > +    ICH9LPCState *lpc = opaque;
> > +    Error *local_err = NULL;
> > +    uint8_t value;
> > +    uint32_t *gcs;
> > +
> > +    visit_type_uint8(v, &value, name, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    value &= ICH9_PS_SPKR_PIN_MASK;
> > +    if (value & ICH9_PS_SPKR_PIN_HIGH) {
> > +        gcs = (uint32_t *)&lpc->chip_config[ICH9_CC_GCS];
> > +        *gcs |= ICH9_CC_GCS_NO_REBOOT;
> > +    }
> > +    lpc->pin_strap.spkr = value;
> > +out:
> > +    error_propagate(errp, local_err);
> > +}
> > +
> >  static void ich9_lpc_add_properties(ICH9LPCState *lpc)
> >  {
> >      static const uint8_t acpi_enable_cmd = ICH9_APM_ACPI_ENABLE;
> >      static const uint8_t acpi_disable_cmd = ICH9_APM_ACPI_DISABLE;
> > +    lpc->pin_strap.spkr = ICH9_PS_SPKR_PIN_DEFAULT;
> >  
> > +    object_property_add(OBJECT(lpc), "pin-spkr", "uint8",
> > +                        ich9_lpc_get_spkr_pin,
> > +                        ich9_lpc_set_spkr_pin,
> > +                        NULL, lpc, NULL);
> >      object_property_add(OBJECT(lpc), ACPI_PM_PROP_SCI_INT,
> > "uint32", ich9_lpc_get_sci_int,
> >                          NULL, NULL, NULL, NULL);
> 
> BTW it's easier to add simple properties in dc->props
> then you don't need all the error propagate code etc.

Hrm - good to know. I'll take a look at it. Thanks.

> 
> 
> > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > index f5681a3..aafc43f 100644
> > --- a/include/hw/i386/ich9.h
> > +++ b/include/hw/i386/ich9.h
> > @@ -46,6 +46,11 @@ typedef struct ICH9LPCState {
> >      ICH9LPCPMRegs pm;
> >      uint32_t sci_level; /* track sci level */
> >  
> > +    /* 2.24 Pin Straps */
> > +    struct {
> > +        uint8_t spkr;
> > +    } pin_strap;
> > +
> >      /* 10.1 Chipset Configuration registers(Memory Space)
> >       which is pointed by RCBA */
> >      uint8_t chip_config[ICH9_CC_SIZE];
> > @@ -72,6 +77,12 @@ Object *ich9_lpc_find(void);
> >  #define Q35_MASK(bit, ms_bit, ls_bit) \
> >  ((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL <<
> > ls_bit) - 1))) 
> > +/* ICH9: Pin Straps */
> > +#define ICH9_PS_SPKR_PIN_LOW        0x01
> > +#define ICH9_PS_SPKR_PIN_HIGH       0x02
> > +#define ICH9_PS_SPKR_PIN_MASK       0x03
> > +#define ICH9_PS_SPKR_PIN_DEFAULT    ICH9_PS_SPKR_PIN_LOW
> > +
> 
> The interface seems a bit inconvenient to me.
> Why not make it a simple boolean property?

No real reason, actually. Since it has no more than 2 states (high and
low), a boolean property would be appropriate. I'll make it boolean.

Thanks,

Paulo

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