Change get-set host command to be less generic

Having a per-device enum list for use by the EC_CMD_GET_SET_VALUE command
won't work when the one-and-only ectool tries to talk to different devices.
Any particular enum may be missing or have a completely different meaning.

Instead, we can do the same thing that EC_CMD_HOST_EVENT_* does - use the
same structs for a bunch of different commands.

If/when we run out of command numbers (it's currently only 8 bits), we'll
just switch to using EC protocol v3 (see crosbug.com/p/20820), which
provides 16 bits for the command.

This CL renames EC_CMD_GET_SET_VALUE to EC_CMD_GSV_PAUSE_IN_S5 (since that's
the one-and-only use of it at present), and renames the params/response
structs as well. Since only the names are changing, the implementation
remains backwards-compatible (assuming the flags value usage is preserved by
ectool for the EC_CMD_GSV_PAUSE_IN_S5 command, which it is).

If I can cherry-pick this change into the one place where it's being used, I
will.

BUG=chromium:287969
BRANCH=ToT
TEST=manual

Although this is primarily an internal name change, it also means that the
commands to invoke the previous usage of this feature have changed. To test:

On Haswell systems only.

To enable the pause in S5 at shutdown, do either of these:

  EC console:      pause_in_s5 on
  root shell:      ectool pause_in_s5 on

Shut the AP down politely, and it should pause in S5 for 10 seconds before
continuing to G3. You can see this by watching the EC console.

To disable the pause in S5 at shutdown, do any of these:

  EC console:      pause_in_s5 off
  root shell:      ectool pause_in_s5 off

or

  press Refresh + POWER

Boot the system, then politely shut down. This time it should go directly to
G3 without pausing in S5.

Change-Id: Ic614fed37ad89db794c2bbcca2b83d1603030ab2
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/168816

7 files changed, 60 insertions, 159 deletions

diff --git a/common/build.mk b/common/build.mk
index 3256982ef0..a31d2c8588 100644
--- a/common/build.mk
+++ b/common/build.mk
@@ -8,7 +8,7 @@
 
 common-y=main.o util.o console_output.o uart_buffering.o
 common-y+=memory_commands.o shared_mem.o system_common.o hooks.o
-common-y+=gpio_common.o version.o printf.o queue.o getset.o
+common-y+=gpio_common.o version.o printf.o queue.o
 common-y+=throttle_ap.o
 
 common-$(BOARD_bolt)+=battery_link.o
diff --git a/common/chipset_haswell.c b/common/chipset_haswell.c
index fcdd00bdc4..f90082508e 100644
--- a/common/chipset_haswell.c
+++ b/common/chipset_haswell.c
@@ -9,9 +9,10 @@
 #include "chipset_x86_common.h"
 #include "common.h"
 #include "console.h"
-#include "getset.h"
+#include "ec_commands.h"
 #include "gpio.h"
 #include "hooks.h"
+#include "host_command.h"
 #include "system.h"
 #include "timer.h"
 #include "util.h"
@@ -50,6 +51,7 @@
 		   IN_PGOOD_ALL_CORE | IN_ALL_PM_SLP_DEASSERTED)
 
 static int throttle_cpu;      /* Throttle CPU? */
+static int pause_in_s5;	      /* Pause in S5 when shutting down? */
 
 void chipset_force_shutdown(void)
 {
@@ -319,7 +321,7 @@ enum x86_state x86_handle_state(enum x86_state state)
 		gpio_set_level(GPIO_PP5000_EN, 0);
 
 		/* Start shutting down */
-		return gsv[GSV_PARAM_s5] ? X86_S5 : X86_S5G3;
+		return pause_in_s5 ? X86_S5 : X86_S5G3;
 
 	case X86_S5G3:
 		/* Deassert DPWROK, assert RSMRST# */
@@ -337,3 +339,35 @@ void haswell_interrupt(enum gpio_signal signal)
 	/* Pass through eDP VDD enable from PCH */
 	gpio_set_level(GPIO_EC_EDP_VDD_EN, gpio_get_level(GPIO_PCH_EDP_VDD_EN));
 }
+
+static int host_command_gsv(struct host_cmd_handler_args *args)
+{
+	const struct ec_params_get_set_value *p = args->params;
+	struct ec_response_get_set_value *r = args->response;
+
+	if (p->flags & EC_GSV_SET)
+		pause_in_s5 = p->value;
+
+	r->value = pause_in_s5;
+
+	args->response_size = sizeof(*r);
+	return EC_RES_SUCCESS;
+}
+DECLARE_HOST_COMMAND(EC_CMD_GSV_PAUSE_IN_S5,
+		     host_command_gsv,
+		     EC_VER_MASK(0));
+
+static int console_command_gsv(int argc, char **argv)
+{
+	if (argc > 1 && !parse_bool(argv[1], &pause_in_s5))
+		return EC_ERROR_INVAL;
+
+	ccprintf("pause_in_s5 = %s\n", pause_in_s5 ? "on" : "off");
+
+	return EC_SUCCESS;
+}
+DECLARE_CONSOLE_COMMAND(pause_in_s5, console_command_gsv,
+			"[on|off]",
+			"Should the AP pause in S5 during shutdown?",
+			NULL);
+
diff --git a/common/getset.c b/common/getset.c
deleted file mode 100644
index 0ac5b840cd..0000000000
--- a/common/getset.c
+++ /dev/null
@@ -1,107 +0,0 @@
-/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-/* Generic get/set value stuff. */
-
-#include "common.h"
-#include "console.h"
-#include "getset.h"
-#include "host_command.h"
-#include "util.h"
-
-/* Declare and initialize the values */
-#define GSV_ITEM(n, v) v,
-#include "getset_value_list.h"
-uint32_t gsv[] = {
-	GSV_LIST
-};
-#undef GSV_ITEM
-
-BUILD_ASSERT(ARRAY_SIZE(gsv) == NUM_GSV_PARAMS);
-
-static enum ec_status get_set_value(struct ec_cmd_get_set_value *ptr)
-{
-	unsigned int index = ptr->flags & EC_GSV_PARAM_MASK;
-	if (index >= NUM_GSV_PARAMS)
-		return EC_RES_INVALID_PARAM;
-
-	/* Handle flags correctly - we may add new ones some day */
-	if (ptr->flags & EC_GSV_SET)
-		gsv[index] = ptr->value;
-
-	ptr->value = gsv[index];
-	return EC_RES_SUCCESS;
-}
-
-static int host_command_get_set_value(struct host_cmd_handler_args *args)
-{
-	const struct ec_cmd_get_set_value *p = args->params;
-	struct ec_cmd_get_set_value *r = args->response;
-	args->response_size = sizeof(*r);
-
-	if (p != r)
-		memcpy(r, p, sizeof(*p));
-
-	return get_set_value(r);
-}
-DECLARE_HOST_COMMAND(EC_CMD_GET_SET_VALUE,
-		     host_command_get_set_value,
-		     EC_VER_MASK(0));
-
-
-#ifdef CONFIG_CMD_GSV
-
-#define STRINGIFY0(name)  #name
-#define STRINGIFY(name)  STRINGIFY0(name)
-#define GSV_ITEM(n, v) STRINGIFY(n),
-#include "getset_value_list.h"
-static const char const *gsv_name[] = {
-	GSV_LIST
-};
-#undef GSV_ITEM
-
-static int console_command_get_set_value(int argc, char **argv)
-{
-	unsigned int i;
-	struct ec_cmd_get_set_value s = {0, 0};
-	char *e;
-	int ret;
-
-	if (argc < 2) {
-		for (i = 0; i < NUM_GSV_PARAMS; i++)
-			ccprintf("%s = 0x%08x\n", gsv_name[i], gsv[i]);
-		return EC_SUCCESS;
-	}
-
-	for (i = 0; i < NUM_GSV_PARAMS; i++)
-		if (!strcasecmp(gsv_name[i], argv[1]))
-			break;
-
-	if (i >= NUM_GSV_PARAMS) {
-		ccprintf("Can't find param \"%s\"\n", argv[1]);
-		return EC_ERROR_UNKNOWN;
-	}
-	s.flags = i;
-
-	if (argc > 2) {
-		s.flags |= EC_GSV_SET;
-		s.value = strtoi(argv[2], &e, 0);
-		if (*e)
-			return EC_ERROR_INVAL;
-	}
-
-	ret = get_set_value(&s);
-
-	if (ret == EC_RES_SUCCESS)
-		ccprintf("%s = 0x%08x\n", argv[1], s.value);
-
-	return ret;
-}
-DECLARE_CONSOLE_COMMAND(gsv, console_command_get_set_value,
-			"[name [value]]",
-			"get/set the value of named parameters",
-			NULL);
-
-#endif	/* CONFIG_CMD_GSV */
diff --git a/include/ec_commands.h b/include/ec_commands.h
index 0287e659ca..5225fc75b5 100644
--- a/include/ec_commands.h
+++ b/include/ec_commands.h
@@ -636,19 +636,30 @@ struct ec_response_get_protocol_info {
 } __packed;
 
 
+/*****************************************************************************/
 /* Get/Set miscellaneous values */
-#define EC_CMD_GET_SET_VALUE	0x0c
 
 /* The upper byte of .flags tells what to do (nothing means "get") */
 #define EC_GSV_SET        0x80000000
-/* The lower three bytes of .flags identifies the parameter */
+
+/* The lower three bytes of .flags identifies the parameter, if that has
+   meaning for an individual command. */
 #define EC_GSV_PARAM_MASK 0x00ffffff
-/* The same struct is sent in both directions */
-struct ec_cmd_get_set_value {
+
+struct ec_params_get_set_value {
 	uint32_t flags;
 	uint32_t value;
 } __packed;
 
+struct ec_response_get_set_value {
+	uint32_t flags;
+	uint32_t value;
+} __packed;
+
+/* More than one command can use these structs to get/set paramters. */
+#define EC_CMD_GSV_PAUSE_IN_S5	0x0c
+
+
 /*****************************************************************************/
 /* Flash commands */
 
diff --git a/include/getset.h b/include/getset.h
deleted file mode 100644
index d1721b3e6c..0000000000
--- a/include/getset.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-/* List of valid params for the generic get/set value operation */
-
-#ifndef __CROS_EC_GETSET_H
-#define __CROS_EC_GETSET_H
-
-#include <stdint.h>
-#include "ec_commands.h"
-
-/* Define the params. */
-#define GSV_ITEM(n, v) GSV_PARAM_##n,
-#include "getset_value_list.h"
-enum gsv_param_id {
-	GSV_LIST
-	NUM_GSV_PARAMS
-};
-#undef GSV_ITEM
-
-/* Declare the storage where the values will be kept. */
-extern uint32_t gsv[];
-
-#endif /* __CROS_EC_GETSET_H */
diff --git a/include/getset_value_list.h b/include/getset_value_list.h
deleted file mode 100644
index b78dc25d14..0000000000
--- a/include/getset_value_list.h
+++ /dev/null
@@ -1,11 +0,0 @@
-/* Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-/**
- * List of get/set value items: <name, init_val>
- */
-#define GSV_LIST \
-	GSV_ITEM(s5, 0) \
-	/* end */
diff --git a/util/ectool.c b/util/ectool.c
index ddf1295baf..9d83d67c31 100644
--- a/util/ectool.c
+++ b/util/ectool.c
@@ -17,7 +17,6 @@
 #include "compile_time_macros.h"
 #include "ec_flash.h"
 #include "ectool.h"
-#include "getset.h"
 #include "lightbar.h"
 #include "lock/gec_lock.h"
 #include "misc_util.h"
@@ -319,23 +318,24 @@ int cmd_test(int argc, char *argv[])
 
 int cmd_s5(int argc, char *argv[])
 {
-	struct ec_cmd_get_set_value s;
+	struct ec_params_get_set_value p;
+	struct ec_params_get_set_value r;
 	int rv;
 
-	s.flags = GSV_PARAM_s5;
+	p.flags = 0;
 
 	if (argc > 1) {
-		s.flags |= EC_GSV_SET;
-		if (!parse_bool(argv[1], &s.value)) {
+		p.flags |= EC_GSV_SET;
+		if (!parse_bool(argv[1], &p.value)) {
 			fprintf(stderr, "invalid arg \"%s\"\n", argv[1]);
 			return -1;
 		}
 	}
 
-	rv = ec_command(EC_CMD_GET_SET_VALUE, 0,
-			&s, sizeof(s), &s, sizeof(s));
+	rv = ec_command(EC_CMD_GSV_PAUSE_IN_S5, 0,
+			&p, sizeof(p), &r, sizeof(r));
 	if (rv > 0)
-		printf("%s\n", s.value ? "on" : "off");
+		printf("%s\n", r.value ? "on" : "off");
 
 	return rv < 0;
 }