| Message ID | 20240418-sbs-time-empty-now-error-v3-1-f286e29e3fca@collabora.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v3] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY_NOW | expand |
Il 18/04/24 19:34, Nícolas F. R. A. Prado ha scritto: > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS > specification as required, it seems that not all batteries implement it. > On platforms with such batteries, reading the property will cause an > error to be printed: > > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5 > > This not only pollutes the log, distracting from real problems on the > device, but also prevents the uevent file from being read since it > contains all properties, including the faulty one. > > The following table summarizes the findings for a handful of platforms: > > Platform Status Manufacturer Model > ------------------------------------------------------------------------ > mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0 > mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J > mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L > mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J > mt8173-elm-hana OK Sunwoda L18D3PG1 > sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K > sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL > rk3399-gru-kevin OK SDI 4352D51 > > Detect if this is one of the quirky batteries during presence update, so > that hot-plugging works as expected, and if so report -ENODATA for > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and > prevents throwing errors. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Nícolas, please, I think that sending this commit to stable for backporting makes a lot of sense since you're actually fixing laptops (that does not really require a Fixes tag) that are supported upstream since .. lots of time ago. In any case, this LGTM. Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi, On Fri, Apr 19, 2024 at 09:26:41AM +0200, AngeloGioacchino Del Regno wrote: > Il 18/04/24 19:34, Nícolas F. R. A. Prado ha scritto: > > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS > > specification as required, it seems that not all batteries implement it. > > On platforms with such batteries, reading the property will cause an > > error to be printed: > > > > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5 > > > > This not only pollutes the log, distracting from real problems on the > > device, but also prevents the uevent file from being read since it > > contains all properties, including the faulty one. > > > > The following table summarizes the findings for a handful of platforms: > > > > Platform Status Manufacturer Model > > ------------------------------------------------------------------------ > > mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0 > > mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J > > mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L > > mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J > > mt8173-elm-hana OK Sunwoda L18D3PG1 > > sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K > > sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL > > rk3399-gru-kevin OK SDI 4352D51 > > > > Detect if this is one of the quirky batteries during presence update, so > > that hot-plugging works as expected, and if so report -ENODATA for > > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and > > prevents throwing errors. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > Nícolas, please, I think that sending this commit to stable for backporting > makes a lot of sense since you're actually fixing laptops (that does not > really require a Fixes tag) that are supported upstream since .. lots of time > ago. > > In any case, this LGTM. > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Considering we have a single commit adding TTE, it should be fine to just add a Fixes tag for that: Fixes: 6ea0126631b0 ("power: supply: sbs-battery: add support for time_to_empty_now attribute") -- Sebastian
On Thu, Apr 18, 2024 at 01:34:23PM -0400, Nícolas F. R. A. Prado wrote: > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS > specification as required, it seems that not all batteries implement it. > On platforms with such batteries, reading the property will cause an > error to be printed: > > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5 > > This not only pollutes the log, distracting from real problems on the > device, but also prevents the uevent file from being read since it > contains all properties, including the faulty one. > > The following table summarizes the findings for a handful of platforms: > > Platform Status Manufacturer Model > ------------------------------------------------------------------------ > mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0 > mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J > mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L > mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J > mt8173-elm-hana OK Sunwoda L18D3PG1 > sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K > sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL > rk3399-gru-kevin OK SDI 4352D51 > > Detect if this is one of the quirky batteries during presence update, so > that hot-plugging works as expected, and if so report -ENODATA for > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and > prevents throwing errors. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- Hi, I'm coming back with more information after some more testing has been done. Most importantly, in the meantime, a parallel investigation uncovered that the time_to_empty_now issue was actually in the EC firmware: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/5465747 So the other faulty properties (which I'll mention below) could also be due to the EC firmware. These are the EC firmware version for the platforms with additional issues: * RW version: juniper_v2.0.2509-9101a0730 * RW version: lazor_v2.0.6519-9923041f79 Hsin-Te, do you have information on whether it's an EC issue in this case as well? The following table shows all the faulty properties per platform: Platform Manufacturer Model Faulty properties --------------------------------------------------------------------------------- mt8186-corsola-steelix-sku131072 BYD L22B3PG0 - mt8195-cherry-tomato-r2 PANASON AP16L5J time_to_empty_now mt8192-asurada-spherion-r0 PANASON AP15O5L time_to_empty_now mt8183-kukui-jacuzzi-juniper-sku16 LGC KT0 AP16L8J time_to_empty_now capacity_error_margin constant_charge_current_max constant_charge_voltage_max current_avg technology manufacture_year manufacture_month manufacture_day SPEC_INFO mt8173-elm-hana Sunwoda L18D3PG1 - sc7180-trogdor-lazor-limozeen-nots-r5 Murata AP18C4K time_to_empty_now capacity_error_margin constant_charge_current_max constant_charge_voltage_max current_avg sc7180-trogdor-kingoftown 333-AC-0D-A GG02047XL time_to_empty_now rk3399-gru-kevin SDI 4352D51 - If it turns out to not be an EC issue for the properties other than the time_to_empty_now, then quirks will need to be added for them. As for SPEC_INFO it's fine to keep it the way it is, as it already fails gracefully by falling back to disabled PEC. However it does mean sbs_update_quirks() would need to be moved up in sbs_update_presence(), or it will never run when SPEC_INFO fails. Also, the battery vendor for limozeen is actually "Murata ", with a trailing space... While at it, I also tested whether PEC was broken on all platforms (which have the SBS battery behind the EC I2C tunnel) to see if it could have any relation with the faulty properties: PEC Platform Manufacturer Model Status ------------------------------------------------------------------------ mt8186-corsola-steelix-sku131072 BYD L22B3PG0 NOT SUPPORTED mt8195-cherry-tomato-r2 PANASON AP16L5J NOT SUPPORTED mt8192-asurada-spherion-r0 PANASON AP15O5L NOT SUPPORTED mt8183-kukui-jacuzzi-juniper-sku16 LGC KT0 AP16L8J NOT SUPPORTED mt8173-elm-hana Sunwoda L18D3PG1 BROKEN sc7180-trogdor-lazor-limozeen-nots-r5 Murata AP18C4K NOT SUPPORTED sc7180-trogdor-kingoftown 333-AC-0D-A GG02047XL NOT SUPPORTED rk3399-gru-kevin SDI 4352D51 BROKEN Where on the platforms marked BROKEN all properties would fail like so: power_supply sbs-9-000b: driver failed to report `status' property: -74 Those platforms indeed had PEC enabled: <6>[ 18.109211] sbs-battery 9-000b: PEC: enabled and I verified the reported SBS version was SBS_VERSION_1_1_WITH_PEC. Meanwhile, all the other platforms, marked NOT SUPPORTED, didn't actually have PEC enabled: <6>[ 14.563070] sbs-battery 8-000b: PEC: disabled which I verified was due to version SBS_VERSION_1_0 being reported (except for jacuzzi, which fails to report a version). So all platforms that had batteries that support PEC, have broken PEC, but most don't support it. In any case there doesn't seem to be a correlation with the properties that the batteries support, so it looks to be an orthogonal issue. Thanks, Nícolas
On Sat, Apr 20, 2024 at 12:03 AM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > On Thu, Apr 18, 2024 at 01:34:23PM -0400, Nícolas F. R. A. Prado wrote: > > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS > > specification as required, it seems that not all batteries implement it. > > On platforms with such batteries, reading the property will cause an > > error to be printed: > > > > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5 > > > > This not only pollutes the log, distracting from real problems on the > > device, but also prevents the uevent file from being read since it > > contains all properties, including the faulty one. > > > > The following table summarizes the findings for a handful of platforms: > > > > Platform Status Manufacturer Model > > ------------------------------------------------------------------------ > > mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0 > > mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J > > mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L > > mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J > > mt8173-elm-hana OK Sunwoda L18D3PG1 > > sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K > > sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL > > rk3399-gru-kevin OK SDI 4352D51 > > > > Detect if this is one of the quirky batteries during presence update, so > > that hot-plugging works as expected, and if so report -ENODATA for > > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and > > prevents throwing errors. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Hi, > > I'm coming back with more information after some more testing has been done. > > Most importantly, in the meantime, a parallel investigation uncovered that the > time_to_empty_now issue was actually in the EC firmware: > https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/5465747 > > So the other faulty properties (which I'll mention below) could also be due to > the EC firmware. These are the EC firmware version for the platforms with > additional issues: > * RW version: juniper_v2.0.2509-9101a0730 > * RW version: lazor_v2.0.6519-9923041f79 > > Hsin-Te, do you have information on whether it's an EC issue in this case as > well? > > The following table shows all the faulty properties per platform: > > Platform Manufacturer Model Faulty properties > --------------------------------------------------------------------------------- > mt8186-corsola-steelix-sku131072 BYD L22B3PG0 - > mt8195-cherry-tomato-r2 PANASON AP16L5J time_to_empty_now > mt8192-asurada-spherion-r0 PANASON AP15O5L time_to_empty_now > mt8183-kukui-jacuzzi-juniper-sku16 LGC KT0 AP16L8J time_to_empty_now > capacity_error_margin > constant_charge_current_max > constant_charge_voltage_max > current_avg > technology > manufacture_year > manufacture_month > manufacture_day > SPEC_INFO > mt8173-elm-hana Sunwoda L18D3PG1 - > sc7180-trogdor-lazor-limozeen-nots-r5 Murata AP18C4K time_to_empty_now > capacity_error_margin > constant_charge_current_max > constant_charge_voltage_max > current_avg > sc7180-trogdor-kingoftown 333-AC-0D-A GG02047XL time_to_empty_now > rk3399-gru-kevin SDI 4352D51 - > > If it turns out to not be an EC issue for the properties other than the > time_to_empty_now, then quirks will need to be added for them. As for SPEC_INFO > it's fine to keep it the way it is, as it already fails gracefully by falling > back to disabled PEC. However it does mean sbs_update_quirks() would need to be > moved up in sbs_update_presence(), or it will never run when SPEC_INFO fails. > > Also, the battery vendor for limozeen is actually "Murata ", with a trailing > space... > > While at it, I also tested whether PEC was broken on all platforms (which have > the SBS battery behind the EC I2C tunnel) to see if it could have any relation > with the faulty properties: > > PEC > Platform Manufacturer Model Status > ------------------------------------------------------------------------ > mt8186-corsola-steelix-sku131072 BYD L22B3PG0 NOT SUPPORTED > mt8195-cherry-tomato-r2 PANASON AP16L5J NOT SUPPORTED > mt8192-asurada-spherion-r0 PANASON AP15O5L NOT SUPPORTED > mt8183-kukui-jacuzzi-juniper-sku16 LGC KT0 AP16L8J NOT SUPPORTED > mt8173-elm-hana Sunwoda L18D3PG1 BROKEN > sc7180-trogdor-lazor-limozeen-nots-r5 Murata AP18C4K NOT SUPPORTED > sc7180-trogdor-kingoftown 333-AC-0D-A GG02047XL NOT SUPPORTED > rk3399-gru-kevin SDI 4352D51 BROKEN > > Where on the platforms marked BROKEN all properties would fail like so: > power_supply sbs-9-000b: driver failed to report `status' property: -74 > > Those platforms indeed had PEC enabled: > <6>[ 18.109211] sbs-battery 9-000b: PEC: enabled > > and I verified the reported SBS version was SBS_VERSION_1_1_WITH_PEC. > > Meanwhile, all the other platforms, marked NOT SUPPORTED, didn't actually have > PEC enabled: > <6>[ 14.563070] sbs-battery 8-000b: PEC: disabled > > which I verified was due to version SBS_VERSION_1_0 being reported (except for > jacuzzi, which fails to report a version). > > So all platforms that had batteries that support PEC, have broken PEC, but most > don't support it. In any case there doesn't seem to be a correlation with the > properties that the batteries support, so it looks to be an orthogonal issue. > > Thanks, > Nícolas It looks like the firmware version of juniper is too old. Could you update the firmware and test it again? Also, Could you provide the error you get from lazor? Regards, Hsin-Te
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c index a6c204c08232..2b1481b81b78 100644 --- a/drivers/power/supply/sbs-battery.c +++ b/drivers/power/supply/sbs-battery.c @@ -214,6 +214,7 @@ struct sbs_info { struct delayed_work work; struct mutex mode_lock; u32 flags; + u32 quirks; int technology; char strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1]; }; @@ -263,6 +264,54 @@ static void sbs_disable_charger_broadcasts(struct sbs_info *chip) dev_dbg(&chip->client->dev, "%s\n", __func__); } +/* Required by the spec, but missing in some implementations */ +#define SBS_QUIRK_BROKEN_TTE_NOW BIT(0) + +struct sbs_quirk_entry { + const char *manufacturer; + const char *model; + u32 flags; +}; + +static const struct sbs_quirk_entry sbs_quirks[] = { + {"PANASON", "AP16L5J", SBS_QUIRK_BROKEN_TTE_NOW}, + {"PANASON", "AP15O5L", SBS_QUIRK_BROKEN_TTE_NOW}, + {"LGC KT0", "AP16L8J", SBS_QUIRK_BROKEN_TTE_NOW}, + {"Murata", "AP18C4K", SBS_QUIRK_BROKEN_TTE_NOW}, + {"333-AC-0D-A", "GG02047XL", SBS_QUIRK_BROKEN_TTE_NOW}, +}; + +static const char *sbs_get_constant_string(struct sbs_info *chip, + enum power_supply_property psp); + +static void sbs_update_quirks(struct sbs_info *chip) +{ + const char *manufacturer; + const char *model; + unsigned int i; + + /* reset quirks from battery before the hot-plug event */ + chip->quirks = 0; + + manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER); + model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME); + if (IS_ERR(manufacturer) || IS_ERR(model)) { + dev_warn(&chip->client->dev, "Couldn't read manufacturer and model to set quirks\n"); + return; + } + + for (i = 0; i < ARRAY_SIZE(sbs_quirks); i++) { + if (strcmp(manufacturer, sbs_quirks[i].manufacturer)) + continue; + if (strcmp(model, sbs_quirks[i].model)) + continue; + chip->quirks |= sbs_quirks[i].flags; + } + + if (chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW) + dev_info(&chip->client->dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n"); +} + static int sbs_update_presence(struct sbs_info *chip, bool is_present) { struct i2c_client *client = chip->client; @@ -323,6 +372,8 @@ static int sbs_update_presence(struct sbs_info *chip, bool is_present) dev_dbg(&client->dev, "PEC: %s\n", (client->flags & I2C_CLIENT_PEC) ? "enabled" : "disabled"); + sbs_update_quirks(chip); + if (!chip->is_present && is_present && !chip->charger_broadcasts) sbs_disable_charger_broadcasts(chip); @@ -614,6 +665,10 @@ static int sbs_get_battery_property(struct i2c_client *client, struct sbs_info *chip = i2c_get_clientdata(client); s32 ret; + if (psp == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW && + chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW) + return -ENODATA; + ret = sbs_read_word_data(client, sbs_data[reg_offset].addr); if (ret < 0) return ret;
Despite the RunTimeToEmpty() (0x11) function being defined in the SBS specification as required, it seems that not all batteries implement it. On platforms with such batteries, reading the property will cause an error to be printed: power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5 This not only pollutes the log, distracting from real problems on the device, but also prevents the uevent file from being read since it contains all properties, including the faulty one. The following table summarizes the findings for a handful of platforms: Platform Status Manufacturer Model ------------------------------------------------------------------------ mt8186-corsola-steelix-sku131072 OK BYD L22B3PG0 mt8195-cherry-tomato-r2 NOT OK PANASON AP16L5J mt8192-asurada-spherion-r0 NOT OK PANASON AP15O5L mt8183-kukui-jacuzzi-juniper-sku16 NOT OK LGC KT0 AP16L8J mt8173-elm-hana OK Sunwoda L18D3PG1 sc7180-trogdor-lazor-limozeen-nots-r5 NOT OK Murata AP18C4K sc7180-trogdor-kingoftown NOT OK 333-AC-0D-A GG02047XL rk3399-gru-kevin OK SDI 4352D51 Detect if this is one of the quirky batteries during presence update, so that hot-plugging works as expected, and if so report -ENODATA for POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and prevents throwing errors. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v3: - Reordered variable declarations and removed unneeded initialization - Link to v2: https://lore.kernel.org/r/20240415-sbs-time-empty-now-error-v2-1-32d8a747e308@collabora.com Changes in v2: - Reworked patch to lay down and use a proper quirk infrastructure, and update the quirks on the presence update callback so it works properly even when hot-plugging different batteries - Link to v1: https://lore.kernel.org/r/20240307-sbs-time-empty-now-error-v1-1-18d0f8702330@collabora.com --- drivers/power/supply/sbs-battery.c | 55 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) --- base-commit: 7b4f2bc91c15fdcf948bb2d9741a9d7d54303f8d change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2 Best regards,