From 1cf94fd052c64ed6aabeed44ea90ee035a77b222 Mon Sep 17 00:00:00 2001 From: Aseda Aboagye Date: Mon, 27 Mar 2017 15:39:36 -0700 Subject: [PATCH] chg_ramp: Don't change ICL until vals are init'd. When the charge ramp task starts, it starts up in the DISCONNECTED state. From this state, it's possible to set the input current limit to 0 mA. However, upon task start, we shouldn't take any action until we have valid values from a new charge supplier or a supplier leaving. This commit changes the charge ramp task to not touch the input current limit until the charge ramp state changes or the desired input current changes at least once. BUG=b:36468002 BRANCH=gru,master TEST=Flash kevin EC RW or build AP FW that contains the fix that will by sync'd by EC SW sync. Make sure WP is asserted to prevent PD communications in RO. Unplug battery. Plug in AC, verify that system can boot up to UI on AC alone. TEST=make -j buildall Change-Id: I351917bce7902c49d1bb842a0cc83dd161d75b6f Reviewed-on: https://chromium-review.googlesource.com/461382 Commit-Queue: Aseda Aboagye Tested-by: Aseda Aboagye Reviewed-by: Randall Spangler Reviewed-on: https://chromium-review.googlesource.com/463927 Commit-Ready: Aseda Aboagye Reviewed-by: Aseda Aboagye --- common/charge_ramp.c | 20 +++++++++++++++----- test/charge_ramp.c | 13 ++++++++++++- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/common/charge_ramp.c b/common/charge_ramp.c index 1c9dfecc33..a5928983fc 100644 --- a/common/charge_ramp.c +++ b/common/charge_ramp.c @@ -159,6 +159,7 @@ void chg_ramp_task(void) static enum chg_ramp_state ramp_st_prev = CHG_RAMP_DISCONNECTED, ramp_st_new = CHG_RAMP_DISCONNECTED; int active_icl_new; + static uint8_t values_have_changed_at_least_once; /* Clear last OCP supplier to guarantee we ramp on first connect */ for (i = 0; i < CONFIG_USB_PD_PORT_COUNT; i++) @@ -322,19 +323,28 @@ void chg_ramp_task(void) task_wait_time = STABLE_VBUS_MONITOR_INTERVAL; break; } - if (ramp_st != ramp_st_new || active_icl != active_icl_new) + if (ramp_st != ramp_st_new || active_icl != active_icl_new) { CPRINTS("Ramp p%d st%d %dmA %dmA", active_port, ramp_st_new, min_icl, active_icl_new); + values_have_changed_at_least_once = 1; + } ramp_st_prev = ramp_st; ramp_st = ramp_st_new; active_icl = active_icl_new; - /* Set the input current limit */ - lim = chg_ramp_get_current_limit(); - board_set_charge_limit(active_port, active_sup, lim, - lim, active_vtg); + /* + * Don't perform any action unless something has changed. + * Otherwise, when the task starts, we may try and set a current + * limit that's invalid/uninitialized. + */ + if (values_have_changed_at_least_once) { + /* Set the input current limit */ + lim = chg_ramp_get_current_limit(); + board_set_charge_limit(active_port, active_sup, lim, + lim, active_vtg); + } if (ramp_st == CHG_RAMP_STABILIZE) /* diff --git a/test/charge_ramp.c b/test/charge_ramp.c index 6e9f239101..29383ad133 100644 --- a/test/charge_ramp.c +++ b/test/charge_ramp.c @@ -121,7 +121,13 @@ static int test_no_ramp(void) system_load_current_ma = 3000; /* A powerful charger, but hey, you're not allowed to ramp! */ plug_charger(CHARGE_SUPPLIER_TEST1, 0, 500, 3000, 3000); - usleep(CHARGE_DETECT_DELAY_TEST); + /* + * NOTE: Since this is currently the first test being run, give the + * charge ramp task enough time to actually transition states and set + * the charge limit. This just needs at least transition to the + * CHG_RAMP_OVERCURRENT_DETECT state. + */ + usleep(CHARGE_DETECT_DELAY_TEST + 200*MSEC); /* That's right. Start at 500 mA */ TEST_ASSERT(charge_limit_ma == 500); TEST_ASSERT(wait_stable_no_overcurrent()); @@ -485,6 +491,11 @@ void run_test(void) { test_reset(); + /* + * If the following test order changes, make sure to add enough time for + * the charge ramp task to make its first transition after plugging in a + * charger. See the comment in test_no_ramp(). + */ RUN_TEST(test_no_ramp); RUN_TEST(test_full_ramp); RUN_TEST(test_vbus_dip);