Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     Samuel Holland <samuel@sholland.org>,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc:     mripard@kernel.org, linux-sunxi@lists.linux.dev,
        linux-kernel@vger.kernel.org, Merlijn Wajer <merlijn@wizzup.org>,
        wens@csie.org
Subject: Re: [BISECTED] Allwinner A33 tablet does not fully power off
Date:   Sun, 06 Nov 2022 08:27:06 +0100
Message-ID: <1909131.PYKUYFuaPT@jernej-laptop>
In-Reply-To: <f02930c0-e471-b3ee-8a3c-6bb745c3ad06@gmail.com>
References: <9f8a0fc3-1d9a-b271-3c26-4f7373b8a3e9@gmail.com> <c1e8a25e-3fd0-ba28-67a5-6775c6ce2d6d@sholland.org> <f02930c0-e471-b3ee-8a3c-6bb745c3ad06@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Dne nedelja, 06. november 2022 ob 08:15:40 CET je Ivaylo Dimitrov napisal(a=
):
> On 5.11.22 =D0=B3. 21:18 =D1=87., Samuel Holland wrote:
> > On 11/5/22 03:23, Ivaylo Dimitrov wrote:
> >> Hi Samuel,
> >>=20
> >> On 5.11.22 =D0=B3. 4:21 =D1=87., Samuel Holland wrote:
> >>> Hi Ivo,
> >>>=20
> >>> On 10/29/22 10:23, Ivaylo Dimitrov wrote:
> >>>> After commit 843107498f91e57d1d4b22cd8787112726fdaeb4 (bus: sunxi-rs=
b:
> >>>> Implement suspend/resume/shutdown callbacks) Q8 A33 tablet I have he=
re
> >>>> cannot be powered-on after power-off, it needs press-and-hold of the
> >>>> power button for 10 seconds (I guess some HW assisted power down
> >>>> happens) before it can be powered-on again.
> >>>>=20
> >>>> The following patch makes it behave correctly:
> >>>>=20
> >>>> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> >>>> index 60b082fe2ed0..30016d62044c 100644
> >>>> --- a/drivers/bus/sunxi-rsb.c
> >>>> +++ b/drivers/bus/sunxi-rsb.c
> >>>> @@ -818,10 +818,7 @@ static int sunxi_rsb_remove(struct platform_dev=
ice
> >>>> *pdev)
> >>>>=20
> >>>>    static void sunxi_rsb_shutdown(struct platform_device *pdev)
> >>>>    {
> >>>>=20
> >>>> -       struct sunxi_rsb *rsb =3D platform_get_drvdata(pdev);
> >>>> -
> >>>>=20
> >>>>           pm_runtime_disable(&pdev->dev);
> >>>>=20
> >>>> -       sunxi_rsb_hw_exit(rsb);
> >>>>=20
> >>>>    }
> >>>> =20
> >>>>    static const struct dev_pm_ops sunxi_rsb_dev_pm_ops =3D {
> >>>>=20
> >>>> I guess the issue comes from the fact that by the time 'power off'
> >>>> command to the power management IC has to be send, the bus it lives =
on
> >>>> is already down, so the device is left in semi-powered down state. O=
fc
> >>>> this is a wild guess, however, preventing the bus being turned off on
> >>>> shutdown fixes the issue.
> >>>=20
> >>> Your guess is correct. The controller gets shut down in
> >>>=20
> >>>     kernel_power_off()
> >>>       kernel_shutdown_prepare()
> >>>         device_shutdown()
> >>>=20
> >>> but the PMIC communication needs to happen later in
> >>>=20
> >>>     kernel_power_off()
> >>>       machine_power_off()
> >>>         pm_power_off()
> >>> =20
> >>>> Please LMK if the above is the correct approach so I will send a pro=
per
> >>>> patch or something else shall be fixed.
> >>>=20
> >>> Yes, this is exactly the right approach. The whole sunxi_rsb_shutdown=
()
> >>=20
> >> Don't we need pm_runtime_disable() on shutdown? As IIUC, the controller
> >> might be suspended and we have to resume it to put it in state to acce=
pt
> >> commands later on(in pm_power_off()).
> >=20
> > sunxi_rsb_write() takes care of resuming the controller, so the
> > controller being suspended prior to pm_power_off() is fine.
> > pm_runtime_disable() would actually prevent resuming the controller
> > later in sunxi_rsb_write().
>=20
> I see.
>=20
> >>> function should be removed. When you send a patch, please add a Fixes:
> >>> tag referencing the commit that you bisected to.
> >=20
> > I found a couple of other issues as well, so I'll send out some fixes
> > with you CC'd.
>=20
> Ok, thanks. What about stable kernels, poweroff is broken since
> 843107498f91e57d1d4b22cd8787112726fdaeb4?

All changes must first hit latest development branch. If they are marked wi=
th=20
fixes tag, they get automatically backported in all currently maintained=20
kernels (mostly LTS) if applicable. But this will take a while.

Best regards,
Jernej

>=20
> Regards,
> Ivo
>=20
> > Regards,
> > Samuel