On Sun, 28 Jun 2015 10:37:58 +0200
"Michael S. Tsirkin" <m...@redhat.com> 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 <pca...@zytor.com>
> > ---
> > 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.
