Re: [PATCH v3 09/11] ARM: dts: imx6ul-kontron-n6x1x-s: Disable the snvs-poweroff driver

[Shawn Guo]

On Mon, Nov 04, 2019 at 11:18:19AM +0000, Schrempf Frieder wrote:
> On 04.11.19 09:06, Frieder Schrempf wrote:
> > Hi Shawn,
> > 
> > On 04.11.19 08:43, Shawn Guo wrote:
> >> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
> >>> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
> >>>
> >>> The snvs-poweroff driver can power off the system by pulling the
> >>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
> >>> The Kontron SoMs do not have this signal connected, so let's remove
> >>> the node.
> >>>
> >>> This seems to fix a real issue when the signal is asserted at
> >>> poweroff, but not actually causing the power to turn off. It was
> >>> observed, that in this case the system would not shut down properly.
> >>
> >> I do not quite follow on this.  How does disabling snvs_poweroff fix the
> >> issue?  The root cause of system not shut down properly seems to be that
> >> PMIC doesn't shut down power.  This looks like a clean-up rather than
> >> bug fix.
> > 
> > I don't know the exact reasons, but we had issues on these boards when 
> > doing a "poweroff". The kernel would print something like the log below.
> > Disabling the snvs-poweroff solved this.
> > 
> > But note that this has last been reproduced on v4.14. So I'm not sure if 
> > this is still a problem with the current kernel.
> > 
> > #######
> > reboot: Power down
> > Unable to poweroff system
> > 
> > ====================================
> > WARNING: halt/675 still has locks held!
> > 4.14.104-exceet #1 Not tainted
> > ------------------------------------
> > 1 lock held by halt/675:
> >   #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
> > #######
> > 
> >>
> >>>
> >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
> >>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron 
> >>> i.MX6UL N6310 SoM and boards")
> >>
> >> If you think this is really a bug fix, it should be applied to the file
> >> before renaming rather than the one after renaming.
> > 
> > I will try to reproduce the issue with the current kernel and depending 
> > on the results either drop the Fixes tag or move the patch before the 
> > renaming.
> 
> I just tried this with 5.4-rc5 and the issue persists. When 
> snvs-poweroff is enabled without the hardware actually being able to 
> power down via PMIC I get a locking WARNING from the kernel. Probably 
> because the system is still running, when it's already supposed to be 
> shut down by the PMIC.

I still do not get it.  Are you saying that with snvs-poweroff being
disabled, your system is now able to be shut down properly by PMIC?

Shawn

> 
> So this fixes a real problem and therefore I will keep the Fixes tag and 
> move the patch to the proper place in this series.