Is it better if we just place the code in system_hibernate defined() defined in the chip we're still using now? (It looks like we won't keep using some ec chips we used before.) The change would be simpler. 

Putting the code in system_hibernate() would work, but rather than duplicating the code for each chip, it would be even better to add a new common hibernate function that does AP shutdown and then calls the chip-level function.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/20c439be209a9cc0bb949ad21f289c453126395f

commit 20c439be209a9cc0bb949ad21f289c453126395f
Author: Philip Chen <philipchen@google.com>
Date: Fri Apr 14 20:49:31 2017

system: Shutdown AP before entering hibernate mode

BUG=chromium:702451
BRANCH=none
TEST=manually test on gru: confirm
'Alt+VolUp+h' puts gru in hibernate mode and
AC plug-in wakes it up.

Change-Id: I3e1134b866dea5d3cc61f9b3dad31c3ff0bd9096
Reviewed-on: https://chromium-review.googlesource.com/470787
Commit-Ready: Philip Chen <philipchen@chromium.org>
Tested-by: Philip Chen <philipchen@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/include/system.h
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/common/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/ish/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/npcx/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/mec1322/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/nrf51/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/host/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/stm32/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/lm4/system.c
[modify] https://crrev.com/20c439be209a9cc0bb949ad21f289c453126395f/chip/it83xx/system.c

This broke hibernate in some situations on Eve because it uses the power button to turn the system off in chipset_force_shutdown, but if it is already off (in S5 but not G3) then it presses the power button to turn it on instead..

I suspect we need to either relax the check in power/skylake/chipset_force_shutdown() to look for CHIPSET_STATE_ANY_OFF or put this behind a config option so it can be disabled on systems that rely on the power button for shutdown.

chipset_force_shutdown also can take a long time to actually turn the system off when it uses the power button so I don't think a 1 second delay is long enough...

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/30bd74b23314f6cd75e1153e3e7be5e19252537f

commit 30bd74b23314f6cd75e1153e3e7be5e19252537f
Author: Duncan Laurie <dlaurie@chromium.org>
Date: Fri Apr 21 13:03:57 2017

Revert "system: Shutdown AP before entering hibernate mode"

This reverts commit 20c439be209a9cc0bb949ad21f289c453126395f.

Reason for revert: This breaks hibernate on skylake boards and
needs to be tested on more than just kevin before submitting.

BUG=chromium:702451
BRANCH=none
TEST=power down and successfully hibernate on Eve

Original change's description:
> system: Shutdown AP before entering hibernate mode
>
> BUG=chromium:702451
> BRANCH=none
> TEST=manually test on gru: confirm
> 'Alt+VolUp+h' puts gru in hibernate mode and
> AC plug-in wakes it up.
>
> Change-Id: I3e1134b866dea5d3cc61f9b3dad31c3ff0bd9096
> Reviewed-on: https://chromium-review.googlesource.com/470787
> Commit-Ready: Philip Chen <philipchen@chromium.org>
> Tested-by: Philip Chen <philipchen@chromium.org>
> Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>
>

TBR=rspangler@chromium.org,aaboagye@chromium.org,philipchen@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
BUG=chromium:702451

Change-Id: Ie847a5e3efb28256b00ddc6534d8ae6bbbba7121
Reviewed-on: https://chromium-review.googlesource.com/482989
Commit-Ready: Duncan Laurie <dlaurie@chromium.org>
Tested-by: Duncan Laurie <dlaurie@chromium.org>
Reviewed-by: Philip Chen <philipchen@chromium.org>
Reviewed-by: Duncan Laurie <dlaurie@chromium.org>

[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/include/system.h
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/common/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/ish/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/npcx/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/mec1322/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/nrf51/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/host/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/stm32/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/lm4/system.c
[modify] https://crrev.com/30bd74b23314f6cd75e1153e3e7be5e19252537f/chip/it83xx/system.c

Re#7
Could you take a look at this patch?
https://chromium-review.googlesource.com/#/c/484724/

I don't have a Eve to test yet.
I'll try to get one next week and then upload the original fix again.

That only fixes one corner case, and potentially introduces another problem because I'm not entirely sure why it is only checking for HARD_OFF in that function...

The other issue with skylake systems is that they rely on the power button for 'chipset_force_shutdown' so the behavior of calling that function can depend on the current state.

In theory that change combined with a much longer timeout (>4 seconds at a minimum, but may need to be longer) in common/system.c might help, but it will need some testing...

Some boards also have their own code to try and shut down before hibernate (like board/reef/board.c:board_hibernate()) which might need to be cleaned up so it doesn't call the same code twice.