linux-kernel - Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 8 Sep 2017 22:28:45 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Liam Breck <liam@...workimprov.net>
Cc:     Sebastian Reichel <sre@...nel.org>,
        Pali Rohár <pali.rohar@...il.com>,
        "Andrew F. Davis" <afd@...com>,
        Linux PM mailing list <linux-pm@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com
Subject: Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

Hi Liam,

> Am 08.09.2017 um 22:19 schrieb Liam Breck <liam@...workimprov.net>:
> 
> Hi Nikolaus,
> 
> On Fri, Sep 8, 2017 at 10:38 AM, H. Nikolaus Schaller <hns@...delico.com> wrote:
>> Hi Liam,
>> I finally continues testing on OpenPandora.
>> 
>>> Am 31.08.2017 um 22:19 schrieb Liam Breck <liam@...workimprov.net>:
>>> 
>>> Hi,
>>> 
>>> This may be a fix that allows >0 input from DT, but won't try to
>>> program the register since the first 3 fields aren't compatible:
>>> 
>>> ... bq27500_dm_regs[] = {
>>> ...
>>> [bq27xxx_dm_design_energy] = {  0,  0, 0,    0, 65535 }, /* missing on chip */
>>> NB: align columns with other rows
>> 
>> I have tried with this DT
>> 
>>        bat: battery {
>>                compatible = "simple-battery", "openpandora-battery";
>>                voltage-min-design-microvolt = <3250000>;
>>                energy-full-design-microwatt-hours = <14800000>;
>>                charge-full-design-microamp-hours = <4100000>;
>>        };
>> 
>> and here is the result:
>> 
>>> root@...ux:~# dmesg|fgrep bq27
>>> [   10.391235] bq27xxx_battery_setup
>>> [   10.391265] bq27xxx_battery_setup: dm_regs=bf0520e0
>>> [   10.393798] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name attribute, please fix
>>> [   10.432678] bq27xxx_battery_settings
>>> [   10.432952] bq27xxx_battery_set_config
>>> [   10.432952] bq27xxx_battery_unseal
>>> [   10.485168] bq27xxx_battery_update_dm_block
>>> [   10.485198] bq27xxx-battery 2-0055: update design-capacity to 4100
>> 
>> looks good
>> 
>>> [   10.485229] bq27xxx_battery_update_dm_block
>>> [   10.485229] bq27xxx-battery 2-0055: buffer does not match design-energy dm spec
>> 
>> ok, ignored
>> 
>>> [   10.511718] bq27xxx_battery_update_dm_block
>>> [   10.511749] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>> 
>> looks good
>> 
>>> [   10.826446] bq27xxx_battery_seal
>>> [   12.150939] bq27xxx_battery_platform_probe
>>> [   12.151031] bq27xxx_battery_setup
>>> [   12.151062] bq27xxx_battery_setup: dm_regs=  (null)
>>> [   12.153411] (NULL device *): hwmon: 'bq27000-battery' is not a valid name attribute, please fix
>>> [   12.154327] bq27xxx_battery_settings
>>> [   12.154357] power_supply bq27000-battery: power_supply_get_battery_info currently only supports devicetree
>>> [   12.154388] bq27xxx_battery_settings: power_supply_get_battery_info failed ret=-6
> 
> Is there also a bq27000 chip on this device, or a battery with it
> embedded? If so, it doesn't appear to have a DT node (correct for
> embedded), hence that warning, which isn't useful in that case. Prob
> battery_settings() should do:
>    if (!di->dev->of_node || power_supply_get_battery_info(...) < 0) return;
> altho that would be wrong once get_battery_info() supports ACPI config...

No it is not available, but could be. The bq27000 can be connected through
HDQ to an OMAP3 and therefore there is no auto-detection. If it is configured
the driver will be loaded. Even if there is no battery_info.

In fact we use the same defconfig for several devices and there is one (GTA04)
which only has a bq27000 inside the battery.

> 
>> ... login:
>> 
>>> root@...ux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>>> POWER_SUPPLY_NAME=bq27500-1-0
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3892000
>>> POWER_SUPPLY_CURRENT_NOW=-317000
>>> POWER_SUPPLY_CAPACITY=0
>> 
>> oops.
>> 
>>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>>> POWER_SUPPLY_TEMP=223
>>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=1147000
>>> POWER_SUPPLY_CHARGE_NOW=2665000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=4100000
>>> POWER_SUPPLY_CYCLE_COUNT=6
>>> POWER_SUPPLY_ENERGY_NOW=0
>>> POWER_SUPPLY_POWER_AVG=64395
>>> POWER_SUPPLY_HEALTH=Dead
>> 
>> oops. But maybe the bq27500 needs a full cycle first
> 
> I wouldn't guess that the above state is due to the DM update
> sequence, but I suppose that's possible. Another update pass might
> clarify that.

Ok, will do tomorrow.

> 
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>> 
>> vvv after plugging in charger
>> 
>>> root@...ux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>>> POWER_SUPPLY_NAME=bq27500-1-0
>>> POWER_SUPPLY_STATUS=Charging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3923000
>>> POWER_SUPPLY_CURRENT_NOW=204000
>>> POWER_SUPPLY_CAPACITY=0
>>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>>> POWER_SUPPLY_TEMP=249
>>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=1147000
>>> POWER_SUPPLY_CHARGE_NOW=2665000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=4100000
>>> POWER_SUPPLY_CYCLE_COUNT=6
>>> POWER_SUPPLY_ENERGY_NOW=0
>>> POWER_SUPPLY_POWER_AVG=800
>>> POWER_SUPPLY_HEALTH=Dead
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>>> root@...ux:~#
>> 
>> Now a reboot after removing charger and battery for several minutes:
>> 
>>> root@...ux:~# dmesg|fgrep bq27
>>> [   10.482818] bq27xxx_battery_setup
>>> [   12.179687] bq27xxx_battery_platform_probe
>>> [   12.179779] bq27xxx_battery_setup
>>> [   12.179809] bq27xxx_battery_setup: dm_regs=  (null)
>>> [   12.182495] (NULL device *): hwmon: 'bq27000-battery' is not a valid name attribute, please fix
>>> [   12.183502] bq27xxx_battery_settings
>>> [   12.183532] power_supply bq27000-battery: power_supply_get_battery_info currently only supports devicetree
>>> [   12.183563] bq27xxx_battery_settings: power_supply_get_battery_info failed ret=-6
>>> [   15.145812] bq27xxx_battery_setup: dm_regs=bf0520e0
>>> [   15.152618] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name attribute, please fix
>>> [   15.346557] bq27xxx_battery_settings
>>> [   15.350585] bq27xxx_battery_set_config
>>> [   15.354553] bq27xxx_battery_unseal
>>> [   15.576538] bq27xxx_battery_update_dm_block
>>> [   15.580993] bq27xxx-battery 2-0055: design-capacity has 4100
>> 
>> ^^^ ok, no change needed
>> 
>>> [   15.676818] bq27xxx_battery_update_dm_block
>>> [   15.681243] bq27xxx-battery 2-0055: buffer does not match design-energy dm spec
>>> [   15.798675] bq27xxx_battery_update_dm_block
>>> [   15.803100] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>> 
>> ^^^ looks as if this is not stored in NVM. Should it be?
> 
> Oh dear. I imagine this is due to a flaw in the multi-block DM update
> logic. It could be that one of the pauses between calls should be
> longer The terminate-voltage spec comes from this datasheet:
> page 19, http://www.ti.com/lit/ds/symlink/bq27500.pdf
> 
> It's possible that this second pass did update it. What does the next
> reboot show?

Will boot tomorrow with exactly the same system and report.

> 
>> BTW: it would be nice if all "update" messages could tell the old value as well.
> 
> We have the orig values in memory since we read the block before
> updating. Feel free to patch that in :-)

Ok.

> 
>>> [   16.011169] bq27xxx_battery_seal
>>> root@...ux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>>> POWER_SUPPLY_NAME=bq27500-1-0
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3894000
>>> POWER_SUPPLY_CURRENT_NOW=-291000
>>> POWER_SUPPLY_CAPACITY=71
>> 
>> ^^^ looks as if the bq27500 did recover from reprogramming
>> 
>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>> POWER_SUPPLY_TEMP=223
>>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=31200
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=3748000
>>> POWER_SUPPLY_CHARGE_NOW=2713000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=4100000
>>> POWER_SUPPLY_CYCLE_COUNT=6
>>> POWER_SUPPLY_ENERGY_NOW=9930000
>>> POWER_SUPPLY_POWER_AVG=64407
>>> POWER_SUPPLY_HEALTH=Good
>> 
>> ^^^ same here
>> 
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>>> root@...ux:~#
> 
> For general use on OpenPandora, you wouldn't have
> config...dt_updates_nvm option set,

Yes.

> so the driver should report what
> the chip settings are and whether they aren't what the DT spec'd.
> Let's verify that too...

Ok.

BR,
Nikolaus