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

[Michael S. Tsirkin]

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.

> and it's sampled low by default.

I think sample high is a safer default.

> 
> 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.


> 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?


>  /* ICH9: Chipset Configuration Registers */
>  #define ICH9_CC_ADDR_MASK                       (ICH9_CC_SIZE - 1)
>  
> -- 
> 2.1.0