Closed exander77 closed 2 years ago
jwise
commented
2 years ago In fast-charge 2 through 4, does setting the input current limit not solve the problem for you? (Set it to, say, $3250 = 3.25A for 65W.) The battery charger should be able to do "smart sharing" between the battery and the CPU. So if the CPU is drawing less power, you get to keep more for the batteries, and if the CPU is drawing more power, then it should scale back battery charging.
exander77
commented
2 years ago If I use anything higher than 1.5-2A, I regularly cycle between charging and not charging. I have a feeling, it may be the same "hickups" that were documented here: https://github.com/exander77/x2100-ec/commit/fc8dbbbd0f0007ff2d92820bf50c0e22de11df01
# 3S3P pack, charge at 12.8V (4.26V/cell), 3.5A, inductor current
# limit is 4.5A so we back off a little to be safe (and also because
# x2100 "hiccups" on charger at 4A) I see some weird behaviour in acpi_listen even when using 1.5A or 2A. But I don't get any notifications about going off charging.
jwise
commented
2 years ago Yeah, those are the 'hiccups'. I don't understand why they should happen: 2A is only 26W of charge power. Where is the rest of the power going??? But yet, it is what it is.
I still wonder if the input current limit would be a better solution than battery charge current limiting. Did you get a chance to try that instead?
exander77
commented
2 years ago @jwise I have no idea what is going on there. But even with 1.5-2A, I see some ACPI messages about going off charging. Maybe there is some kind of detection threshold and when your CPU increases the load, a sudden spike crosses that threshold momentarily? I am not even sure if the battery actually stops charging or if it is just ACPI nonsense reported by EC.
It also spams battery events, I have tried to mitigate the ACPI spam in the: https://github.com/jwise/x2100-ec/pull/8/files#diff-3a6ec67d13623bf8c26e0aa1b40adaf942f9bb2e902a107739f7877a7036a171
But currently, it does some weird problems. Like it does not detect battery inserted. It may need the next set of eyes. Description: https://github.com/jwise/x2100-ec/pull/8
Btw, the patch is big, but I just moved instructions to make space. It is actually just a condition that compares the current value and the previous value as described.
Haven't tested the input current yet.
exander77
commented
2 years ago I think bat_changed_up should be reviewed.
And I love comment: /* Do not do the bad thing unless we've been off the charger for 1f cycles?? */
void __stdcall bat_changed_un(void)
{
word wVar1;
ushort uVar2;
uint in_R0;
undefined3 extraout_var;
undefined3 extraout_var_00;
uint acpi_event;
struct BAT_REG_QUERY *psVar3;
undefined2 *puVar4;
ushort temp_79f04156cdb;
puVar4 = &battery_full_charge_stats_flags;
chargesm_update();
acpi_event = (in_R0 & 0xff00) + (uint)DAT_0001015d;
if ((char)acpi_event != -1) {
acpi_event = (acpi_event & 0xff00) + (uint)(ushort)((char)acpi_event + 1);
DAT_0001015d = (ushort)acpi_event;
}
acpi_event = (acpi_event & 0xff00) + (uint)DAT_00010156;
if ((ushort)acpi_event < 2) {
acpi_event = (acpi_event & 0xff00) + (uint)(ushort)((ushort)acpi_event + 1);
DAT_00010156 = (ushort)acpi_event;
}
else {
DAT_00010156 = 0;
acpi_event = (acpi_event & 0xff00) + (uint)next_bat_query;
if ((char)acpi_event == -1) {
psVar3 = &bat_prop_queries;
}
else {
if ((char)acpi_event != -0x10) goto LAB_000233e0;
psVar3 = &bat_state_queries;
}
next_bat_query = set_bat_query_ptr(psVar3);
acpi_event = (uint)CONCAT31(extraout_var,next_bat_query);
}
LAB_000233e0:
acpi_event = (acpi_event & 0xff00) + (uint)f_n_n_n_n_ONAC_CHGING_DISCH_BATIN;
if (SUB21((*(uint *)(ulong)acpi_event & 0x40) >> 6,0)) {
f_n_n_n_n_ONAC_CHGING_DISCH_BATIN = (ushort)acpi_event & 0xbf;
uVar2 = gpio_read(9,0);
wVar1 = ecreg62_fullcap;
if (SUB21((*(uint *)(CONCAT31(extraout_var_00,uVar2) & 0xffff) & 2) >> 1,0)) {
if (fullcap_raw <= ecreg6c_charge) {
battery_full_charge_stats_flags = battery_full_charge_stats_flags | 1;
}
if (ecreg62_fullcap <= ecreg6c_charge) {
if (max_observed_charge < ecreg6c_charge) {
puVar4[4] = ecreg6c_charge;
*(undefined *)((long)puVar4 + 7) = 0;
}
else {
if (max_observed_charge == ecreg6c_charge) {
if (battery_fully_charged_timer < 0xb) {
*(ushort *)((long)puVar4 + 7) = battery_fully_charged_timer + 1;
}
else {
battery_full_charge_stats_flags = battery_full_charge_stats_flags | 1;
}
}
}
/* nopped out in "true-battery-reading" */
ecreg6c_charge = wVar1;
battery_full_charge_stats_flags = battery_full_charge_stats_flags | 2;
}
if ((bool)((ushort)*(undefined2 *)(ulong)battery_full_charge_stats_flags & 1)) {
do_charge_inhibit_by_battery_req();
if (ecreg6c_charge <= ((fullcap_raw >> 1) / 0x19) * 0x31) {
battery_full_charge_stats_flags = battery_full_charge_stats_flags & 0xfffc;
puVar4[4] = 0;
*(undefined *)((long)puVar4 + 7) = 0;
}
}
else {
do_charge_allow_by_battery_req();
}
if (SUB21((*(uint *)(ulong)battery_full_charge_stats_flags & 2) >> 1,0)) {
uVar2 = 0;
}
else {
uVar2 = 1;
}
}
else {
update_battery_charge_average(ecreg6c_charge);
if (battery_off_charger_timer < 0x1f) {
/* Do not do the bad thing unless we've been off the charger for 1f cycles?? */
*(ushort *)(puVar4 + 3) = battery_off_charger_timer + 1;
}
else {
/* between 70% and 130% of _battery_charge_override */
if ((ecreg6c_charge < (battery_charge_average / 10) * 7) ||
((battery_charge_average / 10) * 0xd < ecreg6c_charge)) {
/* nopped out in "true battery reading" */
ecreg6c_charge = battery_charge_average;
}
}
uVar2 = 2;
}
acpi_event = FUN_000230a6(uVar2);
acpi_event = (acpi_event & 0xff00) + (uint)f_n_n_n_n_ONAC_CHGING_DISCH_BATIN;
if ((bool)((ushort)*(undefined2 *)(ulong)acpi_event & 1)) {
acpi_event = 0x1114;
}
else {
f_n_n_n_n_ONAC_CHGING_DISCH_BATIN = (ushort)acpi_event | 1;
acpi_event = 0x1113;
}
/* Report batery state changed. */
acpi_report(acpi_event,0);
}
FUN_000236e6();
return;
}My patch wrapped acpi_report like this:
if (f_n_n_n_n_ONAC_CHGING_DISCH_BATIN_PREV != (char)uVar1) {
f_n_n_n_n_ONAC_CHGING_DISCH_BATIN_PREV = (char)uVar1;
acpi_report(acpi_event,0);
}@jwise Changing input current works well. I deprecated changing the charging current, added two patches for 65W and 80W.
exander77
commented
2 years ago @jwise Btw, how to use that Makefile.patches?
jwise
commented
2 years ago Try something like:
$ make -f Makefile.patches fast-charge.dump | lua mkpatch.lua That produces a patch that looks very similar to fast-charge.rapatch. There are a few changes in fast-charge.rapatch that differ, you'll note -- the big one is that I add a change to the firmware size field (0x00000018 0065), but also, I reorder the patch to inject the actual jump for the patch only after the code for the patch has been written (not meaningful when baking the patch into ec.bin, yes meaningful when hotpatching the EC!).
exander77
commented
2 years ago @jwise Thank you, seems to work as advertised.
exander77
commented
2 years ago I added some ACPI templates: https://github.com/exander77/x2100-ec/tree/master/acpi
xy-tech
commented
2 years ago Hmu if you guys need someone to test the code!
exander77
commented
2 years ago @xy-tech Feel free to test, I may have finally removed battery ACPI event spam (remove-battery-spam) with some weird solution and I have backported one of the vladissav's patches that I have no idea what it does (vlad00).
jwise
commented
2 years ago Ok, I merged this, and then I just gave you write access to the repo :)
exander77
commented
2 years ago Thank you!
Btw, I would fix the about from "Patches for 51nb X210 Embbeded Controller firmware" to "Patches for 51nb X2100 Embbeded Controller firmware".
Backporting changes from X210 done by vladisslav2011 and adding fan control improvements and other stuff.