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