Re: [Qemu-devel] [PATCH v7 3/3] ich9: implement strap SPKR pin logic
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_LOW0x01
+#define ICH9_PS_SPKR_PIN_HIGH 0x02
+#define ICH9_PS_SPKR_PIN_MASK 0x03
+#define ICH9_PS_SPKR_PIN_DEFAULTICH9_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
Re: [Qemu-devel] [PATCH v7 3/3] ich9: implement strap SPKR pin logic
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_LOW0x01
+#define ICH9_PS_SPKR_PIN_HIGH 0x02
+#define ICH9_PS_SPKR_PIN_MASK 0x03
+#define ICH9_PS_SPKR_PIN_DEFAULTICH9_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
[Qemu-devel] [PATCH v7 3/3] ich9: implement strap SPKR pin logic
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 and it's sampled low by 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);
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_LOW0x01
+#define ICH9_PS_SPKR_PIN_HIGH 0x02
+#define ICH9_PS_SPKR_PIN_MASK 0x03
+#define ICH9_PS_SPKR_PIN_DEFAULTICH9_PS_SPKR_PIN_LOW
+
/* ICH9: Chipset Configuration Registers */
#define ICH9_CC_ADDR_MASK (ICH9_CC_SIZE - 1)
--
2.1.0
