diff mbox series

[v3] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY_NOW

Message ID 20240418-sbs-time-empty-now-error-v3-1-f286e29e3fca@collabora.com
State New
Headers show
Series [v3] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY_NOW | expand

Commit Message

Nícolas F. R. A. Prado April 18, 2024, 5:34 p.m. UTC 
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,

Comments

Nícolas F. R. A. Prado May 9, 2024, 3:25 p.m. UTC | #1 
On Mon, Apr 22, 2024 at 04:10:23PM +0800, Hsin-Te Yuan wrote:
> 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    -
> >
[..]
> 
> 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?

Getting back on this, we were finally able to update the EC firmware for both
juniper and limozeen and all the issues were fixed. I have added the logs below
just for reference. So I guess the only change we could have upstream would be a
message suggesting the user to update the EC firmware in case the SBS is behind
the CrosEC and it starts throwing errors. I'll prepare a patch for that.

Thanks,
Nícolas

limozeen:
+ cat /sys/class/power_supply/sbs-12-000b/time_to_empty_now
3932100
+ cat /sys/class/power_supply/sbs-12-000b/capacity_error_margin
3
+ cat /sys/class/power_supply/sbs-12-000b/constant_charge_current_max
0
+ cat /sys/class/power_supply/sbs-12-000b/constant_charge_voltage_max
0
+ cat /sys/class/power_supply/sbs-12-000b/current_avg
0
+ cat /sys/class/power_supply/sbs-12-000b/uevent
DEVTYPE=power_supply
OF_NAME=sbs-battery
OF_FULLNAME=/soc@0/geniqup@ac0000/spi@a80000/ec@0/i2c-tunnel/sbs-battery@b
OF_COMPATIBLE_0=sbs,sbs-battery
OF_COMPATIBLE_N=1
POWER_SUPPLY_NAME=sbs-12-000b
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_CAPACITY_LEVEL=Full
POWER_SUPPLY_HEALTH=Unknown
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-poly
POWER_SUPPLY_CYCLE_COUNT=2
POWER_SUPPLY_VOLTAGE_NOW=12293000
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_CURRENT_AVG=0
POWER_SUPPLY_CAPACITY=97
POWER_SUPPLY_CAPACITY_ERROR_MARGIN=3
POWER_SUPPLY_TEMP=194
POWER_SUPPLY_TIME_TO_EMPTY_NOW=3932100
POWER_SUPPLY_TIME_TO_EMPTY_AVG=3932100
POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
POWER_SUPPLY_SERIAL_NUMBER=023e
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=11400000
POWER_SUPPLY_VOLTAGE_MAX_DESIGN=11400000
POWER_SUPPLY_ENERGY_NOW=42420000
POWER_SUPPLY_ENERGY_FULL=43700000
POWER_SUPPLY_ENERGY_FULL_DESIGN=51630000
POWER_SUPPLY_CHARGE_NOW=3451000
POWER_SUPPLY_CHARGE_FULL=3555000
POWER_SUPPLY_CHARGE_FULL_DESIGN=4200000
POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=0
POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=0
POWER_SUPPLY_MANUFACTURE_YEAR=2021
POWER_SUPPLY_MANUFACTURE_MONTH=3
POWER_SUPPLY_MANUFACTURE_DAY=14
POWER_SUPPLY_MANUFACTURER=Murata KT00304012
POWER_SUPPLY_MODEL_NAME=AP18C4K
+ cat /sys/class/chromeos/cros_ec/version
RO version:    lazor_v2.0.23149-099cd3e539
RW version:    lazor_v2.0.23149-099cd3e539

juniper:
+ cat /sys/class/power_supply/sbs-12-000b/time_to_empty_now
3932100
+ cat /sys/class/power_supply/sbs-12-000b/capacity_error_margin
3
+ cat /sys/class/power_supply/sbs-12-000b/constant_charge_current_max
0
+ cat /sys/class/power_supply/sbs-12-000b/constant_charge_voltage_max
0
+ cat /sys/class/power_supply/sbs-12-000b/current_avg
0
+ cat /sys/class/power_supply/sbs-12-000b/technology
Li-ion
+ cat /sys/class/power_supply/sbs-12-000b/manufacture_year
2020
+ cat /sys/class/power_supply/sbs-12-000b/manufacture_month
10
+ cat /sys/class/power_supply/sbs-12-000b/manufacture_day
7
+ cat /sys/class/power_supply/sbs-12-000b/uevent
DEVTYPE=power_supply
OF_NAME=sbs-battery
OF_FULLNAME=/soc/spi@11012000/cros-ec@0/i2c-tunnel/sbs-battery@b
OF_COMPATIBLE_0=sbs,sbs-battery
OF_COMPATIBLE_N=1
POWER_SUPPLY_NAME=sbs-12-000b
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_CAPACITY_LEVEL=Full
POWER_SUPPLY_HEALTH=Unknown
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CYCLE_COUNT=8
POWER_SUPPLY_VOLTAGE_NOW=8210000
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_CURRENT_AVG=0
POWER_SUPPLY_CAPACITY=99
POWER_SUPPLY_CAPACITY_ERROR_MARGIN=3
POWER_SUPPLY_TEMP=215
POWER_SUPPLY_TIME_TO_EMPTY_NOW=3932100
POWER_SUPPLY_TIME_TO_EMPTY_AVG=3932100
POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
POWER_SUPPLY_SERIAL_NUMBER=26c9
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=7500000
POWER_SUPPLY_VOLTAGE_MAX_DESIGN=7500000
POWER_SUPPLY_ENERGY_NOW=34990000
POWER_SUPPLY_ENERGY_FULL=35170000
POWER_SUPPLY_ENERGY_FULL_DESIGN=39940000
POWER_SUPPLY_CHARGE_NOW=4262000
POWER_SUPPLY_CHARGE_FULL=4284000
POWER_SUPPLY_CHARGE_FULL_DESIGN=4865000
POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=0
POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=0
POWER_SUPPLY_MANUFACTURE_YEAR=2020
POWER_SUPPLY_MANUFACTURE_MONTH=10
POWER_SUPPLY_MANUFACTURE_DAY=7
POWER_SUPPLY_MANUFACTURER=LGC KT0
POWER_SUPPLY_MODEL_NAME=AP16L8J
+ cat /sys/class/chromeos/cros_ec/version
RO version:    juniper_v2.0.2796-7246101293
RW version:    juniper_v2.0.2796-7246101293
AngeloGioacchino Del Regno May 9, 2024, 3:43 p.m. UTC | #2 
Il 09/05/24 17:25, Nícolas F. R. A. Prado ha scritto:
> On Mon, Apr 22, 2024 at 04:10:23PM +0800, Hsin-Te Yuan wrote:
>> 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    -
>>>
> [..]
>>
>> 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?
> 
> Getting back on this, we were finally able to update the EC firmware for both
> juniper and limozeen and all the issues were fixed. I have added the logs below
> just for reference. So I guess the only change we could have upstream would be a
> message suggesting the user to update the EC firmware in case the SBS is behind
> the CrosEC and it starts throwing errors. I'll prepare a patch for that.
> 

...yes, but then you can't do that in the sbs-battery driver, but rather in the
CrOS EC - so you'd have to link this and the other driver (beware: I'm not
proposing to do that!), which wouldn't be the cleanest of options.

Perhaps we could check "how many times *in a row, from boot*" the readout is
failing and dynamically add the quirk with a big pr_warn().

I guess that could work but, at the same time, that's code to engineer very
carefully, or we'd risk breaking machines that would get that reading to work,
for example, only after a suspend-resume cycle (which is bad, yes, but still)
or other oddities...

Any other ideas?

Cheers,
Angelo


> Thanks,
> Nícolas
> 
> limozeen:
> + cat /sys/class/power_supply/sbs-12-000b/time_to_empty_now
> 3932100
> + cat /sys/class/power_supply/sbs-12-000b/capacity_error_margin
> 3
> + cat /sys/class/power_supply/sbs-12-000b/constant_charge_current_max
> 0
> + cat /sys/class/power_supply/sbs-12-000b/constant_charge_voltage_max
> 0
> + cat /sys/class/power_supply/sbs-12-000b/current_avg
> 0
> + cat /sys/class/power_supply/sbs-12-000b/uevent
> DEVTYPE=power_supply
> OF_NAME=sbs-battery
> OF_FULLNAME=/soc@0/geniqup@ac0000/spi@a80000/ec@0/i2c-tunnel/sbs-battery@b
> OF_COMPATIBLE_0=sbs,sbs-battery
> OF_COMPATIBLE_N=1
> POWER_SUPPLY_NAME=sbs-12-000b
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_STATUS=Full
> POWER_SUPPLY_CAPACITY_LEVEL=Full
> POWER_SUPPLY_HEALTH=Unknown
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-poly
> POWER_SUPPLY_CYCLE_COUNT=2
> POWER_SUPPLY_VOLTAGE_NOW=12293000
> POWER_SUPPLY_CURRENT_NOW=0
> POWER_SUPPLY_CURRENT_AVG=0
> POWER_SUPPLY_CAPACITY=97
> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=3
> POWER_SUPPLY_TEMP=194
> POWER_SUPPLY_TIME_TO_EMPTY_NOW=3932100
> POWER_SUPPLY_TIME_TO_EMPTY_AVG=3932100
> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> POWER_SUPPLY_SERIAL_NUMBER=023e
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=11400000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=11400000
> POWER_SUPPLY_ENERGY_NOW=42420000
> POWER_SUPPLY_ENERGY_FULL=43700000
> POWER_SUPPLY_ENERGY_FULL_DESIGN=51630000
> POWER_SUPPLY_CHARGE_NOW=3451000
> POWER_SUPPLY_CHARGE_FULL=3555000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=4200000
> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=0
> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=0
> POWER_SUPPLY_MANUFACTURE_YEAR=2021
> POWER_SUPPLY_MANUFACTURE_MONTH=3
> POWER_SUPPLY_MANUFACTURE_DAY=14
> POWER_SUPPLY_MANUFACTURER=Murata KT00304012
> POWER_SUPPLY_MODEL_NAME=AP18C4K
> + cat /sys/class/chromeos/cros_ec/version
> RO version:    lazor_v2.0.23149-099cd3e539
> RW version:    lazor_v2.0.23149-099cd3e539
> 
> juniper:
> + cat /sys/class/power_supply/sbs-12-000b/time_to_empty_now
> 3932100
> + cat /sys/class/power_supply/sbs-12-000b/capacity_error_margin
> 3
> + cat /sys/class/power_supply/sbs-12-000b/constant_charge_current_max
> 0
> + cat /sys/class/power_supply/sbs-12-000b/constant_charge_voltage_max
> 0
> + cat /sys/class/power_supply/sbs-12-000b/current_avg
> 0
> + cat /sys/class/power_supply/sbs-12-000b/technology
> Li-ion
> + cat /sys/class/power_supply/sbs-12-000b/manufacture_year
> 2020
> + cat /sys/class/power_supply/sbs-12-000b/manufacture_month
> 10
> + cat /sys/class/power_supply/sbs-12-000b/manufacture_day
> 7
> + cat /sys/class/power_supply/sbs-12-000b/uevent
> DEVTYPE=power_supply
> OF_NAME=sbs-battery
> OF_FULLNAME=/soc/spi@11012000/cros-ec@0/i2c-tunnel/sbs-battery@b
> OF_COMPATIBLE_0=sbs,sbs-battery
> OF_COMPATIBLE_N=1
> POWER_SUPPLY_NAME=sbs-12-000b
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_STATUS=Full
> POWER_SUPPLY_CAPACITY_LEVEL=Full
> POWER_SUPPLY_HEALTH=Unknown
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=8
> POWER_SUPPLY_VOLTAGE_NOW=8210000
> POWER_SUPPLY_CURRENT_NOW=0
> POWER_SUPPLY_CURRENT_AVG=0
> POWER_SUPPLY_CAPACITY=99
> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=3
> POWER_SUPPLY_TEMP=215
> POWER_SUPPLY_TIME_TO_EMPTY_NOW=3932100
> POWER_SUPPLY_TIME_TO_EMPTY_AVG=3932100
> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> POWER_SUPPLY_SERIAL_NUMBER=26c9
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=7500000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=7500000
> POWER_SUPPLY_ENERGY_NOW=34990000
> POWER_SUPPLY_ENERGY_FULL=35170000
> POWER_SUPPLY_ENERGY_FULL_DESIGN=39940000
> POWER_SUPPLY_CHARGE_NOW=4262000
> POWER_SUPPLY_CHARGE_FULL=4284000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=4865000
> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=0
> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=0
> POWER_SUPPLY_MANUFACTURE_YEAR=2020
> POWER_SUPPLY_MANUFACTURE_MONTH=10
> POWER_SUPPLY_MANUFACTURE_DAY=7
> POWER_SUPPLY_MANUFACTURER=LGC KT0
> POWER_SUPPLY_MODEL_NAME=AP16L8J
> + cat /sys/class/chromeos/cros_ec/version
> RO version:    juniper_v2.0.2796-7246101293
> RW version:    juniper_v2.0.2796-7246101293
> _______________________________________________
Nícolas F. R. A. Prado May 13, 2024, 1:27 p.m. UTC | #3 
On Thu, May 09, 2024 at 05:43:42PM +0200, AngeloGioacchino Del Regno wrote:
> Il 09/05/24 17:25, Nícolas F. R. A. Prado ha scritto:
> > On Mon, Apr 22, 2024 at 04:10:23PM +0800, Hsin-Te Yuan wrote:
> > > 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:
[..]
> > 
> > Getting back on this, we were finally able to update the EC firmware for both
> > juniper and limozeen and all the issues were fixed. I have added the logs below
> > just for reference. So I guess the only change we could have upstream would be a
> > message suggesting the user to update the EC firmware in case the SBS is behind
> > the CrosEC and it starts throwing errors. I'll prepare a patch for that.
> > 
> 
> ...yes, but then you can't do that in the sbs-battery driver, but rather in the
> CrOS EC - so you'd have to link this and the other driver (beware: I'm not
> proposing to do that!), which wouldn't be the cleanest of options.

I *was* actually thinking of adding the log in the sbs driver by checking the
parent's compatible, since that's already done elsewhere in that driver to
disable PEC:

	if (of_device_is_compatible(client->dev.parent->of_node, "google,cros-ec-i2c-tunnel")

But now that you mention it, indeed if we're only printing a warning, it would
be best to do it in the EC i2c tunnel driver. And that's all that I'm proposing
to do: log a warning telling the user to update their EC firmware, as that
should fix the readouts, and not add any quirk to the driver.

Thanks,
Nícolas

> 
> Perhaps we could check "how many times *in a row, from boot*" the readout is
> failing and dynamically add the quirk with a big pr_warn().
> 
> I guess that could work but, at the same time, that's code to engineer very
> carefully, or we'd risk breaking machines that would get that reading to work,
> for example, only after a suspend-resume cycle (which is bad, yes, but still)
> or other oddities...
> 
> Any other ideas?
> 
> Cheers,
> Angelo
Alper Nebi Yasak Oct. 3, 2024, 4:32 p.m. UTC | #4 
Hi,

On 2024-05-13 16:27 +03:00, Nícolas F. R. A. Prado wrote:
> On Thu, May 09, 2024 at 05:43:42PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 09/05/24 17:25, Nícolas F. R. A. Prado ha scritto:
>>> On Mon, Apr 22, 2024 at 04:10:23PM +0800, Hsin-Te Yuan wrote:
>>>> 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:
> [..]
>>>
>>> Getting back on this, we were finally able to update the EC firmware for both
>>> juniper and limozeen and all the issues were fixed. I have added the logs below
>>> just for reference. So I guess the only change we could have upstream would be a
>>> message suggesting the user to update the EC firmware in case the SBS is behind
>>> the CrosEC and it starts throwing errors. I'll prepare a patch for that.
>>>
>>
>> ...yes, but then you can't do that in the sbs-battery driver, but rather in the
>> CrOS EC - so you'd have to link this and the other driver (beware: I'm not
>> proposing to do that!), which wouldn't be the cleanest of options.
> 
> I *was* actually thinking of adding the log in the sbs driver by checking the
> parent's compatible, since that's already done elsewhere in that driver to
> disable PEC:
> 
> 	if (of_device_is_compatible(client->dev.parent->of_node, "google,cros-ec-i2c-tunnel")
> 
> But now that you mention it, indeed if we're only printing a warning, it would
> be best to do it in the EC i2c tunnel driver. And that's all that I'm proposing
> to do: log a warning telling the user to update their EC firmware, as that
> should fix the readouts, and not add any quirk to the driver.

I still see this error on a cozmo, even after doing a ChromeOS recovery 
to upgrade EC firmware. (Also, some properties sometimes error with -6). 
Looks like Google did not release an updated version with that patch:

  $ sudo ectool version
  RO version:    cozmo_v2.0.9006-689870d95c
  RW version:    cozmo_v2.0.9006-689870d95c
  Firmware copy: RW
  Build info:    cozmo_v2.0.9006-689870d95c 2022-06-14 10:16:42 @chromeos-ci-firmware-us-central1-b-x32-0-he51

  $ sudo ectool battery
  Battery info:
    OEM name:               PANASON
    Model number:           AP19B5K
    Chemistry   :           LION
    Serial number:          38D5
    Design capacity:        3440 mAh
    Last full charge:       2558 mAh
    Design output voltage   11550 mV
    Cycle count             19
    Present voltage         11607 mV
    Present current         243 mA
    Remaining capacity      2142 mAh
    Flags                   0x06 BATT_PRESENT DISCHARGING

I hope you can ping someone to release a new firmware build (probably 
firmware-icarus-12574.B)? I checked `chromeos-firmwareupdate --manifest` 
as well, but mine matches the version there.

But upgrading firmware would be an ordeal for people who replaced 
ChromeOS with an ordinary Linux distro on the internal disk. ChromeOS 
recovery would wipe their non-ChromeOS system, so they might need to 
figure out how to safely manually upgrade firmware (thinking VPD and A/B 
flags).

Even then, the RO EC firmware will forever carry the EC bug on 
most devices. For example, one of my hana boards goes into a bootloop 
failing to sync EC firmware, where I had to disable that and only 
use RO EC firmware. And we will eventually have non-ChromeOS firmware 
for these devices, which might not properly handle EC firmware.

Please consider fixing it in the kernel as well.
diff mbox series

Patch

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;