Received-SPF: pass (google.com: domain of linux-kernel+bounces-152942-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240418-sbs-time-empty-now-error-v3-1-f286e29e3fca@collabora.com>
 <cf4d8131-4b63-4c7a-9f27-5a0847c656c4@notapiano>
In-Reply-To: <cf4d8131-4b63-4c7a-9f27-5a0847c656c4@notapiano>
From: Hsin-Te Yuan <yuanhsinte@chromium.org>
Date: Mon, 22 Apr 2024 16:10:23 +0800
Message-ID: <CAHc4DNJ0prAQOw89Hvw8n9KhY+8xB3D77pJvoPfU-X7ZFDYu7Q@mail.gmail.com>
Subject: Re: [PATCH v3] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY_NOW
To: =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>
Cc: Sebastian Reichel <sre@kernel.org>, kernel@collabora.com, linux-pm@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Pin-yen Lin <treapking@chromium.org>, 
	Hsin-Te Yuan <yuanhsinte@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 20, 2024 at 12:03=E2=80=AFAM N=C3=ADcolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> On Thu, Apr 18, 2024 at 01:34:23PM -0400, N=C3=ADcolas F. R. A. Prado wro=
te:
> > 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' pr=
operty: -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             L22B3PG=
0
> > 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         L18D3PG=
1
> > sc7180-trogdor-lazor-limozeen-nots-r5   NOT OK  Murata          AP18C4K
> > sc7180-trogdor-kingoftown               NOT OK  333-AC-0D-A     GG02047=
XL
> > rk3399-gru-kevin                        OK      SDI             4352D51
> >
> > Detect if this is one of the quirky batteries during presence update, s=
o
> > 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=C3=ADcolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
>
> Hi,
>
> I'm coming back with more information after some more testing has been do=
ne.
>
> Most importantly, in the meantime, a parallel investigation uncovered tha=
t the
> time_to_empty_now issue was actually in the EC firmware:
> https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/54657=
47
>
> So the other faulty properties (which I'll mention below) could also be d=
ue 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 pr=
operties
> -------------------------------------------------------------------------=
--------
> mt8186-corsola-steelix-sku131072       BYD           L22B3PG0   -
> mt8195-cherry-tomato-r2                PANASON       AP16L5J    time_to_e=
mpty_now
> mt8192-asurada-spherion-r0             PANASON       AP15O5L    time_to_e=
mpty_now
> mt8183-kukui-jacuzzi-juniper-sku16     LGC KT0       AP16L8J    time_to_e=
mpty_now
>                                                                 capacity_=
error_margin
>                                                                 constant_=
charge_current_max
>                                                                 constant_=
charge_voltage_max
>                                                                 current_a=
vg
>                                                                 technolog=
y
>                                                                 manufactu=
re_year
>                                                                 manufactu=
re_month
>                                                                 manufactu=
re_day
>                                                                 SPEC_INFO
> mt8173-elm-hana                        Sunwoda       L18D3PG1   -
> sc7180-trogdor-lazor-limozeen-nots-r5  Murata        AP18C4K    time_to_e=
mpty_now
>                                                                 capacity_=
error_margin
>                                                                 constant_=
charge_current_max
>                                                                 constant_=
charge_voltage_max
>                                                                 current_a=
vg
> sc7180-trogdor-kingoftown              333-AC-0D-A   GG02047XL  time_to_e=
mpty_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 SPE=
C_INFO
> it's fine to keep it the way it is, as it already fails gracefully by fal=
ling
> 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 fa=
ils.
>
> Also, the battery vendor for limozeen is actually "Murata ", with a trail=
ing
> 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 rel=
ation
> with the faulty properties:
>
>                                                                 PEC
> Platform                               Manufacturer  Model      Status
> ------------------------------------------------------------------------
> mt8186-corsola-steelix-sku131072       BYD           L22B3PG0   NOT SUPPO=
RTED
> mt8195-cherry-tomato-r2                PANASON       AP16L5J    NOT SUPPO=
RTED
> mt8192-asurada-spherion-r0             PANASON       AP15O5L    NOT SUPPO=
RTED
> mt8183-kukui-jacuzzi-juniper-sku16     LGC KT0       AP16L8J    NOT SUPPO=
RTED
> mt8173-elm-hana                        Sunwoda       L18D3PG1   BROKEN
> sc7180-trogdor-lazor-limozeen-nots-r5  Murata        AP18C4K    NOT SUPPO=
RTED
> sc7180-trogdor-kingoftown              333-AC-0D-A   GG02047XL  NOT SUPPO=
RTED
> 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 (excep=
t for
> jacuzzi, which fails to report a version).
>
> So all platforms that had batteries that support PEC, have broken PEC, bu=
t 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 is=
sue.
>
> Thanks,
> N=C3=ADcolas

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